refactor(vspec): extract @metadata helper, close line 349 TODO#192
Open
SoundMatt wants to merge 2 commits into
Open
refactor(vspec): extract @metadata helper, close line 349 TODO#192SoundMatt wants to merge 2 commits into
SoundMatt wants to merge 2 commits into
Conversation
…OVESA#8) Issue COVESA#8 raised that VSS allows any datatype for `allowed:` values (e.g. integer values for `Gear: allowed: [1, 2, 3]`) but GraphQL represents enum values as strings, so when the GraphQL exporter emits VSS the datatype information is lost — `vspec.py` line 345 hard-codes `"datatype": "string"`. Extend the existing `@metadata` directive with a new optional `vssDatatype` argument and have the vspec exporter read it from the field's @metadata directive when handling enum-typed fields. Default to "string" when not specified, preserving existing behaviour. Schema authors can then opt in: directive @metadata(comment: String, vssType: String, vssDatatype: String) on FIELD_DEFINITION | OBJECT type Vehicle { gear: GearLevel @metadata(vssType: "sensor", vssDatatype: "int8") } enum GearLevel { _1 _2 _3 _4 _5 } …and the vspec exporter produces: Vehicle.gear: datatype: int8 allowed: - _1 - _2 - _3 - _4 - _5 type: attribute Notes for review: - Issue COVESA#8 closes with "How to handle that?" — this is one reasonable answer; happy to revise the directive shape if you'd prefer a different design (separate @datatype directive on the enum type itself, per-enum-value annotations, derive from value naming convention, etc.). - The metadata-reading code in this branch duplicates the logic used in the scalar-field branch (lines 291–305). Extracting a shared helper would be cleaner; left untouched here to keep the diff scoped to issue COVESA#8. - The other TODO at line 349 ("Get this from the @metadata directive" for the `type:` field) is the same shape of duplication and would benefit from the same refactor; not addressing in this PR. - Tests: the existing `test_export_vspec` end-to-end test still passes (the new code is a no-op without `vssDatatype`). Happy to add a focused unit test asserting the new behaviour if maintainers point me at the right test fixture. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
…or enums Pull the @metadata directive AST-walking logic out of process_field() into a small _read_metadata_args() helper. The scalar and enum branches were doing the same parsing in two places — now both call the helper. While here, close the line 349 TODO in the enum branch by reading 'vssType' from @metadata (falling back to 'attribute' if absent), the same way the scalar branch already does. Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small follow-up to #191. Pulls the
@metadatadirective AST-walking out ofprocess_field()into a_read_metadata_args()helper so the scalar and enum branches stop duplicating the parse. While here, closes the line 349 TODO by having the enum branch readvssTypefrom@metadata(falling back to"attribute"), matching what the scalar branch already does.Net diff: +37 / −35 in a single file. No behaviour change for fields without
@metadata; for enum fields that do have@metadata(vssType: "..."), thetype:line in the YAML now reflects it instead of always being"attribute".Stacked on #191 — please merge that one first. Once #191 lands, the base will need to retarget to
main(or I can rebase + retarget then).Local verification
532 of 538 tests pass. The 6 remaining failures are all
graphql-inspectorCLI tests that need Node installed locally (CI has it).