Solution K (for templates) - #767
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
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. |
|
Documentation preview for this pull request is available at: |
|
Difference (Ai Gened) from #767 and #770 in last commit: They're the same lineage: both branch from 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 Where they diverge Template ownership. #767 hardcodes #770 registers the shipped folder as the default Anchor discovery. This follows directly from the above. #767 computes the expected anchor list in Python ( Filter quoting. #767 pre-builds Template structure. #767's is largely #730's RST with Robustness. #770 adds Scope. #767 touches 8 files, +366/−261, and doesn't update consumer docs. #770 touches 10, +1032/−557, including 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 Both still carry the Two things to watch in #770 specifically. The |
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 Two follow-ups regardless of which lands: document |
|
Can be closed for now too. |
PoC (completely AI generated) just to see how this solution would actually look.