✨ NEW: Add `NeedsPerPageBuilder` #962

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

Create a new builder in builde.py called: NeedsPerPageBuilder
2 . run: make-html-fast -> the need_per_page folder will be gened which has seperated the need with same docs_name

* 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 <daniel.woste@useblocks.com>

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] <support@github.com>

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 <daniel.woste@useblocks.com>

danwos

Some finding, but overall the code looks okay.

However, with Needsfile there is a generic Need-Builder class, which cares about setting everything up the right way.
And I think we should use this and just extend it by a function to store the data in a specific way.
https://github.com/useblocks/sphinx-needs/blob/master/sphinx_needs/needsfile.py

Your code doubles a lot of functions, like filtering, data preparation, and some magic configurations.
That's hard to maintain in the future.

What do you think, is updating Needsfile possible?

danwos · 2023-08-24T07:22:52Z

docs/builders.rst

+
+
+Format with file name: configuration.json
++++++


What does this syntax do?

Please build the docs and check for errors before submitting.
A make docs-html is now enough, as I fixed the doc-build and made it much easier.

danwos · 2023-08-24T07:23:52Z

docs/builders.rst

+
+Format with file name: configuration.json
++++++
+.. code-block:: python


Can you move this to a json file and present it here via .. literalinclude::?
Much easier to maintain.

Maybe just remove this and use the testing snapshots.
The problem with having it "statically" in the documentation, is that it is prone to become out-of-date with the actual code

danwos · 2023-08-24T07:24:49Z

docs/configuration.rst

+
+Build list of ``json`` files that contain all found needs with the same ``docname``. The name of each file should match the ``docname``.
+
+This option works like needs_build_json. 


Please use :ref: to reference other documentation objects.

danwos · 2023-08-24T07:25:54Z

docs/configuration.rst

+
+.. versionadded:: 1.4.0
+
+This option sets the location of list of ``json`` files that contain all found needs with the same ``docname``.


Is this a folder or a file name?
Is it relative to the build folder or to the documentation root?

It is name of folder

danwos · 2023-08-24T07:26:44Z

docs/configuration.rst

+
+This option sets the location of list of ``json`` files that contain all found needs with the same ``docname``.
+
+Default value: need_per_page


Please highlight path information like need_per_page.
Add also a / if it is a folder

danwos · 2023-08-24T07:29:58Z

sphinx_needs/builder.py

+        needs_per_page_dir = os.path.join(self.outdir, needs_per_page_build_path)
+        needs_per_page_data = {}
+        if not os.path.exists(needs_per_page_dir):
+            os.mkdir(needs_per_page_dir)


This does not work, if there are multiple new folders in the path, e.g. needs/pages/examples/.

Use instead:
https://docs.python.org/3/library/os.html#os.makedirs

danwos · 2023-08-24T07:32:57Z

sphinx_needs/builder.py

+            os.mkdir(needs_per_page_dir)
+
+        needs_per_page_data_key = []
+        for need in filtered_needs:


Some more comments would be good, especially if you have such much code in a single function and haven't it broken down to some sub functions.

At least for-loops with multiple lines should get a comment, so that you know in advance what the goal of the for loop is before reading all code line.
Something like sorting needs into docname-based dict would be helpful.

danwos · 2023-08-24T07:39:32Z

sphinx_needs/builder.py

+    format = "needs"
+    file_suffix = ".txt"
+    links_suffix = None
+    LIST_KEY_EXCLUSIONS_NEEDS = ["content_node"]


This is not Builder specific and is shared information that each builder must follow.
So if we need to extend it, we must touch multiple locations. That's not good.

That's one of the reasons why a central ``Needsfile` class was created to deal with such common configs.
https://github.com/useblocks/sphinx-needs/blob/master/sphinx_needs/needsfile.py

tests/test_needs_per_page_builder.py

nhatnamnguyengtvthcm · 2023-08-24T10:37:15Z

Some finding, but overall the code looks okay.

However, with Needsfile there is a generic Need-Builder class, which cares about setting everything up the right way. And I think we should use this and just extend it by a function to store the data in a specific way. https://github.com/useblocks/sphinx-needs/blob/master/sphinx_needs/needsfile.py

Your code doubles a lot of functions, like filtering, data preparation, and some magic configurations. That's hard to maintain in the future.

What do you think, is updating Needsfile possible?

I wonder do you want the format of the JSON file generated the same needs.json. should It generate by function in needsfile?

chrisjsewell · 2023-09-12T09:41:46Z

docs/conf.py

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

+# build json file include needs with the same docs_name
+needs_per_page = True


This option name should be consistent with the other builders (build_json, build_json_per_id), i.e. build_json_per_page

Thinking holistically, with there now being numerous data writers, and more on the way, maybe it makes sense to have some form of unified configuration option.

For example:

needs_data_writers = ["json", "json_per_id", "json_per_page"]

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

LGTM. Approved for contribution.

nhatnamnguyengtvthcm added 6 commits July 14, 2023 14:38

create build for need-per-page

da212be

create build for need-per-page

41b240c

create build for need-per-page

7292694

create build for need-per-page

b175483

update logic write file need json per page

364bcae

add configuration for need per page

59e1b57

This was referenced Aug 21, 2023

♻️ Change NeedsBuilder format to needs #978

Merged

Namnn/need per id json #960

Merged

haiyangToAI and others added 22 commits August 22, 2023 15:32

Removed esbonio for IDE support

fffd402

enable docker arm builds

9054ef7

🔧 Add .nox to gitignore

8277dce

Update pyproject.toml

406f02f

Update pyproject.toml

296cbbd

Update requirements.txt

7d322c6

Update api.rst

240c0ab

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

d336da8

* 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 <daniel.woste@useblocks.com>

🔧 Update pre-commit hooks (useblocks#956)

d1f2c1b

Added config option needs_report_dead_links (useblocks#937)

bfb355c

Co-authored-by: Daniel Woste <daniel.woste@useblocks.com>

Raises version to 1.3.0

5f6bc5d

👌 Performance: Memoize Inline Parser

8d978fa

fix linting

b66b99c

Add to changelog

725d98f

fix merge version 1.3.0

95ea444

Docker fix

0a17349

Removing docker context: ci

25c28c1

♻️ Change NeedsBuilder format to needs

b5b7a3d

Update changelog.rst

76b7442

create build for need-per-page

bbc58df

nhatnamnguyengtvthcm added 10 commits August 22, 2023 15:46

create build for need-per-page

9b514f1

create build for need-per-page

e63e52a

update logic write file need json per page

1c192e0

fix merge version 1.3.0

56c027b

add testcase and changelog.rst and configurations.rst

8332508

add testcase and changelog.rst and configurations.rst

41026f4

add testcase and changelog.rst and configurations.rst

6201bfd

add testcase and changelog.rst and configurations.rst

5770983

add testcase and changelog.rst and configurations.rst

370b026

add testcase and changelog.rst and configurations.rst

95653f4

danwos requested changes Aug 24, 2023

View reviewed changes

update builder and document

4a67fae

chrisjsewell changed the title ~~Namnn/need per page json~~ ✨ NEW: Add NeedsPerPageBuilder Sep 12, 2023

chrisjsewell reviewed Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ NEW: Add `NeedsPerPageBuilder` #962

✨ NEW: Add `NeedsPerPageBuilder` #962

nhatnamnguyengtvthcm commented Aug 4, 2023

danwos left a comment

danwos Aug 24, 2023

danwos Aug 24, 2023

chrisjsewell Sep 12, 2023

danwos Aug 24, 2023

danwos Aug 24, 2023

nhatnamnguyengtvthcm Aug 24, 2023

danwos Aug 24, 2023

danwos Aug 24, 2023

danwos Aug 24, 2023

danwos Aug 24, 2023

nhatnamnguyengtvthcm commented Aug 24, 2023 •

edited

Loading

chrisjsewell Sep 12, 2023

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


		Build list of ``json`` files that contain all found needs with the same ``docname``. The name of each file should match the ``docname``.

		This option works like needs_build_json.


		.. versionadded:: 1.4.0

		This option sets the location of list of ``json`` files that contain all found needs with the same ``docname``.


		This option sets the location of list of ``json`` files that contain all found needs with the same ``docname``.

		Default value: need_per_page

✨ NEW: Add NeedsPerPageBuilder #962

Are you sure you want to change the base?

✨ NEW: Add NeedsPerPageBuilder #962

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhatnamnguyengtvthcm commented Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

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

✨ NEW: Add `NeedsPerPageBuilder` #962

✨ NEW: Add `NeedsPerPageBuilder` #962

nhatnamnguyengtvthcm commented Aug 24, 2023 •

edited

Loading