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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend model classes with small int types, node type and string-based node query #110
base: main
Are you sure you want to change the base?
Extend model classes with small int types, node type and string-based node query #110
Changes from 6 commits
2332a12
0fb2499
70ed6bc
87a4e92
596fca3
1cc87b7
1affb71
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There exists in COVESA VSS also nodes for structs and properties. Any particular reason to limit this one to leaf types, or could we have it more general as
UNKNOWN_TYPE
? (And if so have it first?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we do not support structs and properties.
A general
UNKNOWN_TYPE
could be discussed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having spent some time to think about this: I don't like defining an
unknown
for enums so much, as it always raises the question how to handle this value.I introduced the
UNKNOWN_LEAF_TYPE
just for backwards compatibility reasons, means in case the SDK is used by a model generated before doing these extensions.I've a question regarding your hint regarding structs and properties. At least for structs my understanding is that this isn't a node type but a data type: A struct represents a datapoint/signal of complex nature and can be either of type attribute, sensor, or actuator. I.e. those node types shall not be mixed within a single struct type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the struct/property VSS nodes will never appear in the same tree as signals and branches.
Thanks for the explanation. Possibly add a comment on
UNKNOWN_LEAF_TYPE
that it is there just for backward compatibility reasonsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is branch always returned?
I assume it is because we only instantiate a
Node
forBranch
, in all other case we have a subclass ofNode
that overrides this with something else, or? Maybe worth a comment?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a "pure" Node always represents a branch. I'll add a comment ...