exporters/vspec: support per-field datatype for enum allowed values (#8)#191
Open
SoundMatt wants to merge 1 commit into
Open
exporters/vspec: support per-field datatype for enum allowed values (#8)#191SoundMatt wants to merge 1 commit into
SoundMatt wants to merge 1 commit 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>
Author
|
FYI — opened #192 as a small follow-up that stacks on this branch (extracts the metadata-reading into a helper and closes the line 349 TODO that we discussed in the review). Happy to merge it into this branch first if you'd prefer one combined PR — just let me know. |
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
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.
Summary
Closes #8.
VSS allows any datatype for
allowed:values (e.g. integer-valuedGear: allowed: [1,2,3]), but GraphQL enums are always String atthe schema level, so when the vspec exporter emits VSS the original
datatype information is lost —
src/s2dm/exporters/vspec.py:345hard-codes
"datatype": "string"for any enum-typed field.Approach
Extend the existing
@metadatadirective with a new optionalvssDatatype: Stringargument. The vspec exporter reads it forenum-typed fields. Default
"string"when not specified preservesexisting behaviour for every schema not opting in.
Schema authors
…produces:
Why this approach
The trade-offs I considered before settling on @metadata extension:
@datatypedirective on the enum type (e.g.enum GearLevel @datatype(type: "int8") { ... }) — cleanersemantically (the enum itself is typed) but introduces a new
vocabulary. I went with extending @metadata to keep one
directive-per-concept and avoid bikeshedding the new directive's
name and target locations.
uniform across all values of an enum.
_followed by a digit ⇒ int) — too brittle; works only fornumeric enums, breaks for
["DIESEL","E10","E5"]style.The directive-arg approach is the smallest reachable change that
solves the problem without bikeshedding new vocabulary or breaking
any existing schema.
Notes for review
reasonable answer; happy to revise the directive shape if
you'd prefer something different.
branch's logic (vspec.py:291–305). Extracting a shared helper
would be cleaner; left untouched here to keep the diff scoped
to issue Handle datatypes for allowed values #8.
"Get this from the @metadata directive"for thetype:field) is the same shape ofduplication and would benefit from the same refactor; not
addressing in this PR.
test_export_vspecend-to-end test still passes(the new code is a no-op without
vssDatatype). Happy to add afocused unit test asserting the new behaviour if you can point
me at the right test fixture pattern.