✨ NEW: Add `NeedsLookUpTableBuilder` #961

nhatnamnguyengtvthcm · 2023-08-04T09:32:32Z

add builder called NeedsLookUpTableBuilder
Builder NeedsLookUpTableBuilder generates needs_lut.json file include {id: docs_name_path}

danwos

Thanks for the PR.
The idea is good and the implementation is promising.
Unfortunately, I have some findings:

I'm missing any documentation of the new builder in the buiders.rst file.
This should also contain an example of how the final file looks like.

Also a test case is missing for the javascript part.
You can find examples of it inside /tests/js_test:
https://github.com/useblocks/sphinx-needs/tree/master/tests/js_test

danwos · 2023-08-14T15:18:59Z

docs/conf.py

@@ -380,6 +380,9 @@ def custom_defined_func():
 # build needs.json to make permalinks work
 needs_build_json = True

+# build needs_lut.json to make permalinks work
+needs_lut_build_json = True


I think this is not needed for our Sphinx-Needs docs?

docs/configuration.rst

danwos · 2023-08-14T15:20:57Z

docs/configuration.rst

+.. __needs_lut_build_json:
+
+needs_lut_build_json
+~~~~~~~~~~~~~~~~~~~~~~~


headline too long

docs/configuration.rst

sphinx_needs/builder.py

danwos · 2023-08-14T15:28:33Z

sphinx_needs/needs.py

@@ -279,6 +284,7 @@ def setup(app: Sphinx) -> Dict[str, Any]:
    #
    app.add_config_value("needs_debug_measurement", False, "html", types=[dict])

+    app.add_config_value("needs_permalink_url", None, "html")


Where is this used and documented?

sphinx_needs/templates/permalink.html

* Added config allow unsafe filter for filter_func * Fixed review * Updated sphinx-immaterial to fix CI * Fix linkcheck anchor issue --------- Co-authored-by: Daniel Woste <[email protected]>

Bumps [actions/checkout](https://github.com/actions/checkout) from 3.3.0 to 3.5.3. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3.3.0...v3.5.3) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>

Inside the LaTeX generator, it looks for the attributes ids and refid for creating link targets. When a new layer with a <container> wrapper was added in commit e51fc1f, for an unknown reason this triggered these additional IDs to generate links within the LaTeX output. A test case has been added to ensure only one hyperref within LaTeX is encountered as well. (useblocks#808)

Co-authored-by: Daniel Woste <[email protected]>

danwos

This PR needs to be updated to the latest changes for 1.4.0 (master).
Especially the way of option registration has changed.

some other minor findings.

danwos · 2023-08-31T04:51:35Z

docs/builders.rst

+---------
+.. versionadded:: 1.4.0
+
+The **needs_lut** builder exports all found needs to a single json file, which only include list of key ``id`` and value of ``docname`` or ``external_url``.


We should mention what lut stands for. Maybe:

**needs_lut** (Needs Lookup Table)

danwos · 2023-08-31T04:51:58Z

docs/builders.rst

+
+The **needs_lut** builder exports all found needs to a single json file, which only include list of key ``id`` and value of ``docname`` or ``external_url``.
+
+The build creates file called **needs_lut.json** inside the given build-folder.


.. a file called

danwos · 2023-08-31T04:56:38Z

sphinx_needs/builder.py

+            fname = os.path.join(self.outdir, "needs_lut.json")
+            with open(fname, "w", encoding="utf-8") as f:
+                json.dump(needs_dict, f, indent=4)
+        except Exception as e:


Can we add this part (dict creation + json.dump) also to NeedsList as write_json_lut?
It's easier to maintain if all storage functions are defined in one place.

I think we should stay with add_need (instead of add_lut_need) and transform the needed data in a special writer function write_json_lut.

chrisjsewell

Heya @nhatnamnguyengtvthcm, as you can see in #1019 I added snapshot testing, to provide snapshots of the expected data.

Perhaps you could add this to your test_doc_needs_lut_builder

As an example, see:

sphinx-needs/tests/test_needs_builder.py

Line 14 in 09277e3

assert needs_list == snapshot(exclude=props("created"))

chrisjsewell · 2023-09-12T09:52:42Z

sphinx_needs/config.py

@@ -252,6 +252,7 @@ def __setattr__(self, name: str, value: Any) -> None:
    # add config for needs_id_builder
    build_json_per_id: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})
    build_json_per_id_path: str = field(default="needs_id", metadata={"rebuild": "html", "types": (str,)})
+    lut_build_json: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})


Suggested change

lut_build_json: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})

build_json_lut: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})

This would be consistent with build_json and build_json_per_id

danwos

Thankas for the changes.
I think the builder is more complex as it should be, just see my comment.

I also like the snapshot idea from Chris for the tests.

danwos · 2023-09-13T05:09:47Z

sphinx_needs/needsfile.py

@@ -131,6 +131,9 @@ def load_json(self, file: str) -> None:

            self.log.debug(f"needs.json file loaded: {file}")

+    def add_lut_need(self, version: str, needs_lut: Dict[str, Any]) -> None:


My idea was a little different.
Instead of adding a custom data structure and use the same writer-function, we could keep the data structure with add_need() and just do the output specific formating in a new writer function like write_lut_json.
In this case all the code is the same, till it comes to write it to the harddisk.

I wonder if we keep formatting JSON and only map need["docname"] = need["external_url"] if need["is_external"] = True, It is easy to reuse flow generate need_lut.json like need.json. Otherwise, I will continue change formatting in write_lut_json like {[id] : [docname] or [external_url]}
@r-o-b-e-r-t-o @danwos

@nhatnamnguyengtvthcm Not sure if I got your point. But I think we must have a mapping for internal and external needs because otherwise you would not be redirected correctly by permalink.html in case an external need is referenced.

danwos · 2023-09-13T05:11:44Z

sphinx_needs/builder.py

+            fname = os.path.join(self.outdir, "needs_lut.json")
+            with open(fname, "w", encoding="utf-8") as f:
+                json.dump(needs_dict, f, indent=4)
+        except Exception as e:


I think we should stay with add_need (instead of add_lut_need) and transform the needed data in a special writer function write_json_lut.

r-o-b-e-r-t-o · 2023-09-14T09:40:58Z

LGTM. Approved for contribution.

Instead of using `filter_needs()` for a single need (`id == ""XY`) , the need is taken directly from `all_needs`, so that we do not need to perform any filtering.

nhatnamnguyengtvthcm added 11 commits July 18, 2023 13:34

add builder needs look up for permalink

bdc4d77

keep variable for need_file

37deab7

keep variable for need_file

4095a0d

change some env variable

b144bc3

add config mode permalink

0af5175

add config mode permalink

c9a10fd

add config mode permalink

db592c0

add config mode permalink

99dc2dd

fix ci test

9452708

update confiration for needs_lut

9d6badf

update confiration for needs_lut

adfc885

danwos requested changes Aug 14, 2023

View reviewed changes

add test-case and fix some setting for need per id builder

02e65be

chrisjsewell mentioned this pull request Aug 21, 2023

♻️ Change NeedsBuilder format to needs #978

Merged

update version

5199edc

chrisjsewell mentioned this pull request Aug 21, 2023

Namnn/need per id json #960

Merged

haiyangToAI and others added 14 commits August 22, 2023 13:23

Removed esbonio for IDE support

d4e4f04

enable docker arm builds

390dd36

🔧 Add .nox to gitignore

7177c03

Update pyproject.toml

61dcbef

Update pyproject.toml

98030a8

Update requirements.txt

25d42f0

Update api.rst

994b6b1

Added config allow unsafe filter for filter_func (useblocks#949)

4f5d185

* Added config allow unsafe filter for filter_func * Fixed review * Updated sphinx-immaterial to fix CI * Fix linkcheck anchor issue --------- Co-authored-by: Daniel Woste <[email protected]>

🔧 Update pre-commit hooks (useblocks#956)

78aedd7

Added config option needs_report_dead_links (useblocks#937)

c6f0c2d

Co-authored-by: Daniel Woste <[email protected]>

Raises version to 1.3.0

4babb2d

👌 Performance: Memoize Inline Parser

4cec93b

danwos and others added 12 commits August 22, 2023 13:24

Docker fix

036c723

Removing docker context: ci

d3e0f90

♻️ Change NeedsBuilder format to needs

e144909

Update changelog.rst

d229036

add builder needs look up for permalink

4ead31a

fix ci test

c6723ea

update confiration for needs_lut

d0e7849

add test-case and fix some setting for need per id builder

2faeec3

fix merge version 1.3.0

3d32679

fix merge version 1.3.0

de90ff2

fix merge version 1.3.0

569d5a6

update format builder

830c12a

danwos requested changes Aug 31, 2023

View reviewed changes

chrisjsewell changed the title ~~Namnn/needs lookup~~ ✨ NEW: Add NeedsLookUpTableBuilder Sep 7, 2023

nhatnamnguyengtvthcm added 3 commits September 11, 2023 16:38

merge needsidbuilder

a93f75f

turn off needs_lut_build_json

ff74834

change test case name

b69da59

chrisjsewell requested changes Sep 12, 2023

View reviewed changes

chrisjsewell reviewed Sep 12, 2023

View reviewed changes

danwos requested changes Sep 13, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into Namnn/needs-lookup

27bd083

nhatnamnguyengtvthcm and others added 4 commits September 18, 2023 20:10

refix needs look up builder, test_case

c77162c

👌 Optimize needextend filter_needs usage (useblocks#1030)

5fdcd3c

Instead of using `filter_needs()` for a single need (`id == ""XY`) , the need is taken directly from `all_needs`, so that we do not need to perform any filtering.

change logic write_lut_json and test case

fd1f612

git commit -m"fix conflict changelog.rst"

6649155

nhatnamnguyengtvthcm force-pushed the Namnn/needs-lookup branch from 0796b94 to 6649155 Compare September 25, 2023 10:54

nhatnamnguyengtvthcm and others added 3 commits September 25, 2023 17:55

Merge branch 'master' into Namnn/needs-lookup

c3bb82c

fix-confict change log for needs_lut_builder

0a20d81

refactor filter_needs in need_lut_builder

4472aed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ NEW: Add `NeedsLookUpTableBuilder` #961

✨ NEW: Add `NeedsLookUpTableBuilder` #961

nhatnamnguyengtvthcm commented Aug 4, 2023

danwos left a comment

danwos Aug 14, 2023

danwos Aug 14, 2023

danwos Aug 14, 2023

danwos left a comment

danwos Aug 31, 2023

danwos Aug 31, 2023

danwos Aug 31, 2023

danwos Sep 13, 2023

chrisjsewell left a comment

chrisjsewell Sep 12, 2023

danwos left a comment

danwos Sep 13, 2023

nhatnamnguyengtvthcm Sep 21, 2023

r-o-b-e-r-t-o Sep 22, 2023 •

edited

Loading

danwos Sep 13, 2023

r-o-b-e-r-t-o commented Sep 14, 2023


		The needs_lut builder exports all found needs to a single json file, which only include list of key ``id`` and value of ``docname`` or ``external_url``.

		The build creates file called needs_lut.json inside the given build-folder.

	lut_build_json: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})
	build_json_lut: bool = field(default=False, metadata={"rebuild": "html", "types": (bool,)})

		@@ -131,6 +131,9 @@ def load_json(self, file: str) -> None:

		self.log.debug(f"needs.json file loaded: {file}")

		def add_lut_need(self, version: str, needs_lut: Dict[str, Any]) -> None:

✨ NEW: Add NeedsLookUpTableBuilder #961

Are you sure you want to change the base?

✨ NEW: Add NeedsLookUpTableBuilder #961

Conversation

nhatnamnguyengtvthcm commented Aug 4, 2023

danwos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisjsewell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-o-b-e-r-t-o Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-o-b-e-r-t-o commented Sep 14, 2023

✨ NEW: Add `NeedsLookUpTableBuilder` #961

✨ NEW: Add `NeedsLookUpTableBuilder` #961

r-o-b-e-r-t-o Sep 22, 2023 •

edited

Loading