Feat/generate ubproject and schema validation#394
Conversation
…score#19) by: Aymen Soussi aymen.soussi@expleogroup.com
* Fixed source_code_linker finding external needs * Added explaining comments Added simple loop search logic to try all available prefixes per id.
Include inc files in doc build
Signed-off-by: Maximilian Sören Pollak <maximilian.pollak@expleogroup.com>
- Added target for building Github archive - Take the input files an put them in the archive as-is. No renaming. - Use empty lists for now, as we are still not sure how to use or adapt the source code linker in multirepo Addresses: eclipse-score#16 --------- Signed-off-by: Dan Calavrezo <dan.calavrezo.ext@qorix.ai>
updated version for release Addresses: eclipse-score#16 Signed-off-by: Dan Calavrezo <dan.calavrezo.ext@qorix.ai>
* Fixed wrong check activation * Fixed test & rst files Test and rst files needed fixing to comply with new check rules Check found False positives due to 'process' being moved and loosing it's prefix.
Cleaned up unused imports and small fix inside the README Also-by: Aymen Soussi aymen.soussi@expleogroup.com
- Add multiple 'uni-direcional' docs build examples - Bugfix 'docs_needs' build target in Module imports - Patch some spelling mistakes - Delete 'docs'. It has been moved to examples/simple already. --- Addresses a bug that caused the 'docs_needs' import via a Module (like in the linking-release example) to not be executed correctly, as it would be missing the dependencies and missing the 'sphinx_build binary'
Signed-off-by: Alexander Lanin <Alexander.Lanin@etas.com>
* Add bugfix for new module json_encoding quirks
Remove defined external 'id_prefixes' from to be checked links. Added another example to have an rst file inside a folder as well. Added some explanation to descriptions of needs
* Increase version for release
trustable framework needs, eclipse-score/process_description#27 add security tag to the document need Resolves: eclipse-score/score#947
Signed-off-by: Nicolae Dicu <nicolae.dicu.ext@qorix.ai>
Signed-off-by: Andrey Babanin <andrey.babanin@bmw.de>
Co-authored-by: Maximilian Sören Pollak <maximilian.pollak@expleogroup.com>
so far score was hardcoded
… of using a temporary file
…com/arnoox/score-docs-as-code into feat/generate-sn6-schema-validation
…alidity period consistency Co-authored-by: Copilot <copilot@github.com>
…emporary files Co-authored-by: Copilot <copilot@github.com>
…into feat/generate-sn6-schema-validation
…com/arnoox/score-docs-as-code into feat/generate-sn6-schema-validation
There was a problem hiding this comment.
I tested this and it looks functionally working well. Here are some finding, a mix of AI, and my personal findings.
Found 2 issues:
-
The error message in
_classify_linkssays only the first regex will be used when multiple regex patterns are supplied for the same link field, but the implementation unconditionally overwrites on each iteration so the last regex actually wins. The companion test even asserts the last value is kept (# Last regex overwrites previous ones), confirming the message is wrong.docs-as-code/src/extensions/score_metamodel/sn_schemas.py
Lines 110 to 120 in 5f1263b
Test that confirms the actual (last-wins) behavior:
docs-as-code/src/extensions/score_metamodel/tests/test_sn_schemas.py
Lines 155 to 163 in 5f1263b
Fix: either change the message to say "the last one will be used", or
continueafter logging so the first one is actually retained (and update the test accordingly). -
needs_metamodel_generated.tomlshould not be written intoconfdir._generate_needs_types_toml,_generate_needs_fields_tomland_generate_needs_links_tomlbuild TOML strings purely in memory, but the setup then writes the concatenation toPath(app.confdir) / "needs_metamodel_generated.toml"solely so the path can be appended toneedscfg_merge_toml_files.needs_config_writerthen merges the same content intoubproject.toml, so the on-disk file contains nothing the finalubproject.tomldoesn't already have — and it lands inside the user's source tree (docs/), forcing a.gitignoreentry. Runningbazel run //:docs_checkproduces both files side-by-side with overlapping contents, which is confusing and serves no consumer.docs-as-code/src/extensions/score_sync_toml/__init__.py
Lines 154 to 167 in 5f1263b
.gitignoreentry added to hide the generated file (also covers the redundancy):README explicitly labels the file as an "Intermediate merge input for
needs_config_writer":Fix options (in order of preference):
- Write the intermediate to
app.outdir(or atempfile.mkdtemp()directory cleaned up via abuild-finishedhandler) so it never appears underconfdir. The.gitignoreentry can then be removed. - Better still: extend
needs_config_writerto accept an in-memory TOML fragment (string or dict) so the intermediate file is not needed at all. The current "deterministic path so no temp file leak" rationale (__init__.py:157) is solving the wrong problem — the leak only exists becauseconfdirwas chosen as the target.
- Write the intermediate to
Lower-confidence findings (below the 80-point bar)
The 5 parallel review agents surfaced these additional candidates. Each was scored 0–100 by an independent agent; only items at ≥80 are reported as confirmed issues above. The rest are listed here for completeness — they are either latent (no current trigger), intentional, or pre-existing.
2. Optional-field regex alternation ^$|{pattern} (score: 60)
In get_pattern_schema, optional fields wrap the metamodel pattern as f"^$|{pattern}". JSON Schema's pattern keyword is defined as a substring search (ECMA-262 semantics, not full match), whereas the post-build Python checker uses re.fullmatch. For patterns that are not fully ^…$ anchored (e.g. ^https://github.com/.* used in source_code_link / testlink fields in metamodel.yaml), the alternation ^$|^https://github.com/.* may behave differently in jsonschema-rs (used by sphinx-needs / ubCode) than in the Python checker, allowing values one engine accepts and the other rejects.
docs-as-code/src/extensions/score_metamodel/sn_schemas.py
Lines 300 to 320 in 5f1263b
Why scored 60: The current metamodel mostly uses fully anchored patterns, so this is a real but latent inconsistency rather than a bug hitting today. Worth a TODO and a test for an unanchored pattern.
3. Missing TOML escaping in _generate_needs_types_toml (score: 60)
_generate_needs_fields_toml and _generate_needs_links_toml apply .replace("\\", "\\\\").replace('"', '\\"') before interpolating values into TOML strings. _generate_needs_types_toml does not — directive, title, and prefix are written raw. If any need-type title or prefix ever contains a backslash or double quote, the generated TOML will be syntactically invalid and downstream parsing will fail.
Why scored 60: Real divergence from the sibling helpers, but no current metamodel value contains the problem characters, so the bug is latent. Easy to fix by mirroring the escape calls used in the field/link helpers.
4. Default ID pattern broadened to allow uppercase (score: 65)
The auto-generated fallback ID pattern in yaml_parser.py widened from ^{prefix}[0-9a-z_]+$ (lowercase only) to ^{prefix}[0-9a-zA-Z_]+$ (mixed case). This affects every need type that does not declare an explicit id: pattern.
docs-as-code/src/extensions/score_metamodel/yaml_parser.py
Lines 130 to 140 in 5f1263b
Why scored 65: The change appears to have ridden in on an earlier sphinx-needs 6.3.0 upgrade commit on the same branch (not introduced in this PR's primary diff hunks), and there is no commit message or comment explaining the policy shift. Confirm intentional; if so, document; if not, restore lowercase.
5. needscfg_exclude_vars may duplicate needs_config_writer defaults (score: 25)
In score_sync_toml/__init__.py, the setup code does list(getattr(app.config, "needscfg_exclude_vars", None) or []) + ["needs_from_toml", "needs_from_toml_table", "needs_schema_definitions_from_json"]. If needs_config_writer (loaded earlier in the extension order) already registers those three entries as the default value, the result will contain duplicates.
Why scored 25: Could not confirm needs_config_writer actually registers these defaults; even if duplicates appear, exclusion-list duplication is typically benign. Worth a quick check during review.
6. Optional-link validate.network intentionally omitted (score: 5 — false positive)
sn_schemas.py deliberately skips emitting validate.network for optional links, with an in-source explanation: the post-build Python checker reports optional-link target-type violations at treat_as_info=True, while sphinx-needs schema validators only support a single violation severity per schema rule. Including optional links in the schema would escalate info-level findings to hard errors. A possible solution is to write optional links into dedicated rules, so they can be assigned a dedicated severity. The README's coverage table documents this trade-off.
Why scored 5: Documented intentional design decision with rationale and tests. Not an issue.
Follow-up suggestions (not blocking this PR)
These are improvements to track in subsequent PRs — they are not regressions in #394, but the surrounding code makes them natural next steps.
F1. Replace empty-string sentinels with nullable fields
_generate_needs_fields_toml always writes default = "" for any field that has no metamodel-supplied default. Throughout the codebase this empty string is then re-interpreted as "field not given" in later checks. Modelling absence with nullable fields (null / Optional) is the cleaner alternative — distinguishing a deliberately empty value from an unset one — and avoids the readability and type-safety problems that come with overloaded empty strings.
docs-as-code/src/extensions/score_sync_toml/__init__.py
Lines 20 to 43 in 5f1263b
F2. Promote shared enum values to field-level schemas
Where a field carries the same enum across every need type that uses it, the per-type duplication can be hoisted into a single [needs.fields.<name>] schema entry, e.g.
[needs.fields.status]
schema.enum = ["draft", "review", "approved", "done"]This would shrink the generated schemas.json, make ubCode autocompletion show enum values for the field everywhere it appears, and keep the metamodel DRY. _generate_needs_fields_toml currently emits only default — extending it with schema.enum (and possibly schema.type) when the same constraint holds across every type that uses the field is the natural follow-up.
docs-as-code/src/extensions/score_sync_toml/__init__.py
Lines 20 to 43 in 5f1263b
F3. Emit $schema header in generated ubproject.toml
The generated ubproject.toml should start with
"$schema" = "https://ubcode.useblocks.com/ubproject.schema.json"so that editors with TOML schema support give schema-aware completion and validation while editing the file. See ubCode configuration docs. This would likely live as a top-of-file fragment in shared.toml or as a needs_config_writer option, depending on how the merge tool orders keys.
F4. Convert needs_schema_debug_path to a relative path
The generated ubproject.toml currently contains an absolute, machine-specific path:
schema_debug_path = "/home/marco/git/eclipse-score/docs-as-code/docs/schema_debug"needs_config_writer already supports rewriting absolute paths via needscfg_relative_path_fields, and its own docstring explicitly names needs_schema_debug_path as the example field for this option:
"List of config path patterns to convert to relative paths. Can be strings (e.g.,
'needs_schema_debug_path') ..."
—needs_config_writer/main.py:94
The fix is to add "needs_schema_debug_path" to the existing needscfg_relative_path_fields.extend([...]) call. If sphinx-needs cannot consume a relative value for this field, that is an upstream bug to file against sphinx-needs / needs-config-writer; otherwise this is a one-line change.
docs-as-code/src/extensions/score_sync_toml/__init__.py
Lines 168 to 177 in 5f1263b
ubmarco
left a comment
There was a problem hiding this comment.
I am approving this PR, the noted review comments can be addressed in follow-up PRs. Functionally all good.
|
Don't we test that in CI? 😨 |
| @@ -0,0 +1,234 @@ | |||
| # score_metamodel | |||
There was a problem hiding this comment.
I don't think it makes sense to create this README here which seems to be AI slop more or less duplicating info from our actual documentation.
There was a problem hiding this comment.
fair point. I will remove this.
| # Decision Records | ||
| affects: | ||
| incoming: affected by | ||
| outgoing: affects |
There was a problem hiding this comment.
Why is the metamodel extended here? Was that a gap before? Why didn't we find that before then?
There was a problem hiding this comment.
I was just reviewing that. It was a gap. Why we didnt find that ourselves is a very valid question.
| /_build* | ||
| docs/ubproject.toml | ||
| docs/schemas.json | ||
| docs/needs_metamodel_generated.toml |
There was a problem hiding this comment.
Even more files getting generated into the workspace?
This clashes with my proposal here: eclipse-score/score#2826
Technically, this is just a draft, so not really relevant for this PR though. Just want to mention it.
There was a problem hiding this comment.
docs/needs_metamodel_generated.tomlcan definitely be moved to some build dir. I'll fix as part of this PR.docs/schemas.jsonis maybe required in the workspace. I can check if it is possible forubproject.tomlto reference to something from within the build dir.
| class TestGetPatternSchema: | ||
| def test_returns_string_type_with_pattern(self) -> None: | ||
| result = get_pattern_schema("^[A-Z]+$") | ||
| assert result == {"type": "string", "pattern": "^[A-Z]+$"} |
There was a problem hiding this comment.
We should trim down the worst of this AI generated stuff. Some tests here are not really useful.
There was a problem hiding this comment.
Agreed, the tests for the trivial dict building helpers add no real value. I will keep only the tests that cover actual branching logic.
| def test_valid_need_passes( | ||
| self, schemas_by_type: dict[str, dict[str, Any]] | ||
| ) -> None: | ||
| assert_schema_valid(self._make_valid(), schemas_by_type["feat_req"]) |
There was a problem hiding this comment.
Same as in the other test file: Some tests are pointless and should be removed.
| # This enables sphinx-needs 6 schema validation: required fields, regex | ||
| # patterns on option values, and (eventually) link target type checks. | ||
| # Use config-inited event to defer file writing until config is ready, | ||
| # with error handling to prevent boot crashes on file write failures. |
There was a problem hiding this comment.
Shouldn't his be a docstring? Comment before method makes no sense.
…s-code into feat/generate-sn6-schema-validation
…github.com/arnoox/score-docs-as-code into feat/generate-sn6-schema-validation
|
I'm not so sure about the architecture here. score_metamodel writes the json, and the toml is written by a separate extension. However the toml has a hard dependency on having a json file. Don't the toml and the json also overlap in their content? |
|
Can we figure out some common naming scheme for the generated files? With this change we'll need to touch gitignore for all users, so some wildcard pattern would be nice. |
📌 Description
🚨 Impact Analysis
✅ Checklist