Update static UID fka functionality. #326
Merged
Conversation
This file contains 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
Collaborator
adobekan
commented
Jan 25, 2024
•
adobekan
commented
- improve backward compatibility
- include strict mode = case sensitive for the semantic check
- include additional tests
* to lowercase before patch Signed-off-by: Adnan Bekan <[email protected]> * Binary parser bug fix. Signed-off-by: Ulf Bjorkengren <[email protected]> --------- Signed-off-by: Adnan Bekan <[email protected]> Signed-off-by: Ulf Bjorkengren <[email protected]> Co-authored-by: Adnan Bekan <[email protected]> Co-authored-by: Ulf Bjorkengren <[email protected]>
* to lowercase before patch Signed-off-by: Adnan Bekan <[email protected]> * Binary parser bug fix. Signed-off-by: Ulf Bjorkengren <[email protected]> --------- Signed-off-by: Adnan Bekan <[email protected]> Signed-off-by: Ulf Bjorkengren <[email protected]> Co-authored-by: Adnan Bekan <[email protected]> Co-authored-by: Ulf Bjorkengren <[email protected]>
* add strict mode for case sensitivity Signed-off-by: Niclas Wesemann <[email protected]> * update documentation Signed-off-by: Niclas Wesemann <[email protected]> * fix trailing whitespace Signed-off-by: Niclas Wesemann <[email protected]> --------- Signed-off-by: Niclas Wesemann <[email protected]>
Signed-off-by: Niclas Wesemann <[email protected]>
Signed-off-by: Niclas Wesemann <[email protected]>
* to lowercase before patch Signed-off-by: Adnan Bekan <[email protected]> * Binary parser bug fix. Signed-off-by: Ulf Bjorkengren <[email protected]> --------- Signed-off-by: Adnan Bekan <[email protected]> Signed-off-by: Ulf Bjorkengren <[email protected]> Signed-off-by: Niclas Wesemann <[email protected]> Co-authored-by: Adnan Bekan <[email protected]> Co-authored-by: Ulf Bjorkengren <[email protected]>
Signed-off-by: Adnan Bekan <[email protected]>
Signed-off-by: Adnan Bekan <[email protected]>
Signed-off-by: Niclas Wesemann <[email protected]>
erikbosch
reviewed
Jan 26, 2024
docs/vspec2id.md
Outdated
| [-vt vspec_types_file] [-ot <types_output_file>] [--json-all-extended-attributes] [--json-pretty] [--yaml-all-extended-attributes] [-v version] [--all-idl-features] | ||
| [--gqlfield GQLFIELD GQLFIELD] [--validate-static-uid VALIDATE_STATIC_UID] [--only-validate-no-export] | ||
| [-q quantity_file] [-vt vspec_types_file] [-ot <types_output_file>] [--json-all-extended-attributes] [--json-pretty] [--yaml-all-extended-attributes] [-v version] [--all-idl-features] | ||
| [--gqlfield GQLFIELD GQLFIELD] [--jsonschema-all-extended-attributes] [--jsonschema-disallow-additional-properties] [--jsonschema-require-all-properties] [--jsonschema-pretty] |
There was a problem hiding this comment.
We could potentially remove some fields that have no impact (and will not even be allowed after my vss-tools restructuring PR has been merged) like [--yaml-all-extended-attributes] and [--gqlfield GQLFIELD GQLFIELD]
erikbosch
reviewed
Jan 26, 2024
| @@ -93,14 +97,14 @@ warnings only: | |||
| | Data type | | Deprecation | | |||
| | Type (i.e. node type) | | Deleted Attribute | | |||
| | Unit | | Change description | | |||
| | Enum values (allowed) | | | | |||
| | Enum values (allowed) | | Qualified name (fka) | | |||
There was a problem hiding this comment.
Maybe a stupid question but what does "fka" means
erikbosch
reviewed
Jan 26, 2024
| @@ -43,6 +45,9 @@ cd path/to/your/vss-tools | |||
| Great, you generated your first overlay that will also be used as your validation file as soon as you update your | |||
| vehicle signal specification file. | |||
|
|
|||
| If needed you can make the static UID generation case-sensitive using the command line argument `--strict-mode`. It | |||
| will default to false. | |||
There was a problem hiding this comment.
What does case sensitive mean here? That two signals A.B and A.b get different UID so that renaming A.b to A.B is considered a name change?
erikbosch
reviewed
Jan 26, 2024
| @@ -78,11 +83,10 @@ A.B.NewName: | |||
| type: actuator | |||
| allowed: ["YES", "NO"] | |||
| description: A.B.NewName's old name is 'OldName'. And its even older name is 'OlderName'. | |||
| fka: ['A.B.OldName', 'A.B.OlderName'] | |||
| fka: ['A.B.OlderName', 'A.B.OldName'] | |||
There was a problem hiding this comment.
Does the order have any significance?
|
Looks good in general, I had some minor comments. The PR history is quite messy with merge commits (and cherry-picks?). I suggest that the PR author does a local squash and rebase on latest master and do a force update of the branch before merge, so that we can keep a clean history on master. |
* check if fka is a list Signed-off-by: Adnan Bekan <[email protected]> * update documentation Signed-off-by: Adnan Bekan <[email protected]> --------- Signed-off-by: Adnan Bekan <[email protected]>
|
MoM: Decision: Merge when maintainer happy |
erikbosch
pushed a commit
that referenced
this pull request
May 3, 2024
* add strict mode for case sensitivity Signed-off-by: Niclas Wesemann <[email protected]> Signed-off-by: Adnan Bekan <[email protected]>
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.