Skip to content

Solution K (for templates) - #767

Closed
MaximilianSoerenPollak wants to merge 5 commits into
eclipse-score:mainfrom
MaximilianSoerenPollak:MSP_solution_k
Closed

Solution K (for templates)#767
MaximilianSoerenPollak wants to merge 5 commits into
eclipse-score:mainfrom
MaximilianSoerenPollak:MSP_solution_k

Conversation

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor

PoC (completely AI generated) just to see how this solution would actually look.

⚠️ This SHOULD NOT BE MERGED AS IS (even if it works)

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //src:license-check

Status: ⚠️ Needs Review

Click to expand output
[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: e217a281-f2f8-4f8f-8067-8826648ba3ab
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: src
WARNING: Target pattern parsing failed.
ERROR: Skipping '//src:license-check': no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
ERROR: no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
INFO: Elapsed time: 5.688s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor Author

Self critique of the clanker:

What's right

The load-bearing decision is intact: parse_text_to_nodes(..., allow_section_headings=True) with the nodes returned into the surrounding document, Need as first sibling. And the reason is written into both the module docstring and the README as "do not clean this up," which is exactly the guardrail I was worried about, since moving the body inside the Need node breaks it invisibly.

setup() has no lifecycle hooks. No consistency_checks.py, no registry, no init/purge/merge. The one config-inited handler is registration-only and there's a test (test_extension_has_no_build_lifecycle_hooks) asserting that stays true. That's better than my brief asked for.

rendering.py is pure string transformation with the governing rule at the top, and it holds — nothing in there receives a Need. Filter interpolation goes through quote_for_filter with an allow-list plus json.dumps, and there's a hostile-ID test. Coverage/LCOV and the Bazel macro are absent, as intended.

The metamodel check is bidirectional and lives in score_metamodel/checks/, picked up by discover_checks(). It also correctly filters covers to comp targets only, since covers can legitimately point at requirements.

Two things it did better than my brief

It reused the existing mod_ver_report type in metamodel.yaml rather than adding a link field — covers, contains, evidence were already declared there. So the "make :components: a link field" work item was mostly already done upstream and the PR just uses it.

derive_title is demoted to an explicit last-resort fallback with a :titles: override, and the docstring says the real title comes from the :need: reference. That's the right framing; I'd only said "allow an override."

_promote_report_anchors is a genuine addition. Docutils appends .. _name: ids after the heading-derived one, so section["ids"][0] — what Sphinx uses for the ToC entry and the HTML id — would have been the unstable, non-namespaced one with a -1 suffix on collision. Fixing that at read time is correct and it's why the stable-anchor and two-reports tests pass.

What I'd raise

The ReportNeedDirective alias is the one piece of real cleverness, and it deserves scrutiny. Because the public mod_ver_report name is taken by the report directive, the generated RST calls mod_ver_report_need, and the subclass resets self.name to restore the type. It works, but it depends on NeedDirective deriving its type from the invoked name — an implementation detail of sphinx-needs. It also leaks a second public directive name into the namespace. Worth a comment pinning the sphinx-needs version, and worth checking whether needs_types registration or a differently-named public directive avoids the shadowing entirely.

The priority-900 config-inited ordering against sphinx-needs' 500 is undocumented coupling. If sphinx-needs ever moves its registration, this silently reverts to a plain Need and every section disappears. That failure is invisible in HTML — same category as the sibling issue. Add an assertion or a test that the report directive is the one actually resolved.

_merge_target_into_section mutates document.ids / document.nameids and removes the target so PropagateTargets doesn't double-merge. Correct as far as I can tell, but it's the most docutils-version-sensitive code in the PR. Pin it in the test matrix.

Minor: the README lists :covers: still being ANY in the yaml as a known gap — agreed, narrowing it is consumer-affecting and belongs in a separate PR. And "two reports at different heading depths on one page" being untested is worth closing, since UNDERLINE uses a single char in a fresh title-style context and that's the assumption doing the work.

Net: I'd move it out of draft after the directive-shadowing question is answered. The SHOULD NOT BE MERGED AS IS label is fair for a PoC, but the structure is the one to build on rather than restart.

@github-actions

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-767: https://eclipse-score.github.io/docs-as-code/pr-767/

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor Author

Difference (Ai Gened) from #767 and #770 in last commit:

They're the same lineage: both branch from 79b4ec4, then each has exactly one commit on top. #767's is "use jinja"; #770's is "WIP: Solution K 2nd path" — a second pass over the same idea. So #770 isn't an alternative design, it's #767's move carried further.

The shared move

Both take the report body out of Python string constants and put it in a Jinja template rendered from explicit directive inputs. Both keep the governing rule intact — no Need reaches the renderer. And both port back the missing needpie charts and grid layout from #730, so the content gap you spotted is closed in each.

Where they diverge

Template ownership. #767 hardcodes mod_ver_report_tiny.need at parents[2]/needs_templates with its own FileSystemLoader. Private file, private lookup, no way for a project to change it.

#770 registers the shipped folder as the default needs_template_folder in setup(), then searches the project's folder first and falls back to shipped. So a consumer overrides the entire report by dropping their own mod_ver_report.need next to their other need templates. That's the substantive difference — it turns the report from a fixed page into a customizable one, using sphinx-needs' existing template mechanism rather than a parallel one.

Anchor discovery. This follows directly from the above. #767 computes the expected anchor list in Python (rendering.section_anchors(report_id, ids)), which only works if Python knows the section structure. #770 regex-scans the rendered text for .. _<report_id>__*: targets and promotes whatever it finds. Once the template is overridable, Python can't know the structure — so scanning is the correct consequence, not just a refactor.

Filter quoting. #767 pre-builds evidence_filter and scope_filter strings in Python and passes them in. #770 registers quote_for_filter as a Jinja filter ({{ feature_id|q }}), so a template author writing their own filter expression still gets safe quoting. Again: needed only because templates became user-editable.

Template structure. #767's is largely #730's RST with {{ }} substituted — repetitive, 237 lines. #770 factors it into macros (heading, status_pie, coverage_pie, inspection_pie, requirements_table, architecture_table, workproduct_table) and drives work products from two list constants. Longer at 299 lines, but the repetition is gone and the macros are the extension point for an overriding template.

Robustness. #770 adds {% if feature_id %} / {% else %}No feature declared and an empty-components branch. #767 assumes both exist. Since feature_id is derived by string substitution (mod__feat__), a module without a matching feature produces empty pies in #767 and an explicit message in #770.

Scope. #767 touches 8 files, +366/−261, and doesn't update consumer docs. #770 touches 10, +1032/−557, including module_verification_report.md (+58), the example RST (+105), and roughly 500 lines of added tests.

Assessment

#770 is better, and for one reason that matters more than the others: making the report a real sphinx-needs template means a consumer who wants a different report edits RST instead of filing an issue against docs-as-code. Everything else — anchor scanning, the q filter, the macros — is downstream of that decision and correct given it.

Both still carry the mod_ver_report_need shadowing trick and the priority-900 registration, so that open question is unchanged.

Two things to watch in #770 specifically. The q filter is opt-in — a template author who writes "{{ component.id }}" in satisfied_by instead of {{ component.id|q }} gets no protection, so that needs to be loud in the docs. And project-first template resolution means a stale local mod_ver_report.need silently shadows an upgraded shipped one; worth a debug-level log saying which folder won.

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor Author
Aspect PR #767 (use jinja) PR #770 (Solution K 2nd path)
Base commit 79b4ec4 + 1 commit 79b4ec4 + 1 commit (same base)
Diff size 8 files, +366 / −261 10 files, +1032 / −557
Report body Jinja template mod_ver_report_tiny.need Jinja template mod_ver_report.need
Template lookup Hardcoded parents[2]/needs_templates, private loader Registered as needs_template_folder; project folder searched first, shipped as fallback
Consumer can override report? No Yes — drop your own mod_ver_report.need
Anchor promotion Python computes expected anchor list (section_anchors) Regex-scans rendered text for .. ___*: targets
Filter quoting Python pre-builds scope_filter / evidence_filter q Jinja filter (`{{ id
Template structure #730's RST with {{ }} substituted; repetitive (237 lines) Macro-based: heading, status_pie, coverage_pie, inspection_pie, requirements_table, architecture_table, workproduct_table (299 lines)
Missing feature / no components Assumed present; renders empty pies Explicit {% if %} fallbacks with a message
needpie charts + grid Ported from #730 Ported from #730
Consumer docs updated No Yes (module_verification_report.md +58, example +105)
Test additions test_rendering −64 test_rendering +238, integration +275
Env caching New Environment per render lru_cache on environment

Recommendation: #770. The overridable template is the difference that matters — it decides whether changing a report is an RST edit or an upstream PR. Anchor scanning, the q filter, and the macros are all consequences of that choice rather than independent preferences.

Two follow-ups regardless of which lands: document |q as mandatory for anything interpolated into a filter, and log which template folder won so a stale local override isn't invisible.

@MaximilianSoerenPollak

Copy link
Copy Markdown
Contributor Author

Can be closed for now too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants