-
Notifications
You must be signed in to change notification settings - Fork 57
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
Core Rework ⚠️ BREAKING CHANGES #382
Core Rework ⚠️ BREAKING CHANGES #382
Conversation
7aa37eb
to
50ad785
Compare
3718d7b
to
ac50f5a
Compare
24184fc
to
ddf228b
Compare
MoM:
|
3e9d96e
to
e060807
Compare
if csv_field is None: | ||
csv_field = "" | ||
formatted_csv_line = ( | ||
formatted_csv_line + '"' + str(csv_field).replace('"', '""') + '",' |
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.
I notice some changes in output of CSV exporters. This line was added once upon the time to handle at least some common scenarios, like if a description/comment contain a comma or a double quote, and then the string has to be quoted according to https://datatracker.ietf.org/doc/html/rfc4180
Now it seems we skip any form of escaping, or?
(Or are you compensating it by adding some stricter checks, like an error if someone use a comma in a description?, or just removing such characters.)
An alternative solution could possibly be to only use "default escaping" only for free text fields like description and comments.
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.
I think the csv writer library handles escaping for us. And yes, the output of all csv's changed and removed unnecessary quoting
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.
But I will check some corner cases I guess
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.
Would be quite easy to add some test cases unless we already have some.
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.
It works fine. I honestly see no real reason to test the official csv lib
5c76cd2
to
2deb3f8
Compare
d599b92
to
7b71f20
Compare
ea137af
to
178295c
Compare
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
178295c
to
1e2f458
Compare
MoM: #Questions We are never exporting the arraysize attribute. On purpose?S Schildt: Not on purpose vss2binary -> export_node -> validate what is it?@UlfBj to be the one to answer.
Should type tree properties be compliant to name-style and should follow the same --strict/--aborts name-style behavior?
|
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
From my perspective, it is done |
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Changed how includes are resolved since it was non deterministic. Now building a complete list of all vspec files first. Merging all of them afterwards. Changed that dict values are deeply updated. Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
This information is already linked to: |
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
I am wondering why it is not a specified internal attribute if it is present in the code and explicitly mentioned? |
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
set does not guarantee an order. However an order is crucial for something like include dirs a user can specify. Files could exist in two include dirs. Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Signed-off-by: Sebastian Schleemilch <sebastian.schleemilch@bmw.de>
Because it is "only" used in VISS (and thus the binary exporter, which currently is also only for the VISSR reference implementation), and not considered part of VSS (i.e. expected to be supported by all VSS tooling). That does not mean it is "not good", it is just similar to e.g. DBC mapping keys (currently afaik only used in KUKSA) or some specific deployment keys BMW might use in internal tooling. I think, what would be a nice addition for the future, in case a specific tooling "requires" or understands special keys (like the binary exporter), if it could programmatically via an API whitelist those, instead of forcing the user to whitelist them on the command line. |
Meeting 20th: Looking good, What about the breaking changes?
MoM: Merge |
Fixes: #380
Fixes: #390
Fixes the issue in discussion of #360 (removed the test in test_overlay_on_instance)
Rewrote the core logic using Pydantic.
overlaying of instance nodes without the need to specify nodes
completely
datatypes, units, allowed attributes etc
-e/--extended-attributes
to not be able to pass core attributesTip
Overlay nodes do not need any attributes anymore except the ones you want to overwrite
That is also valid for overwriting instances.
Example:
Warning
This breaks the backwards compatibility to older vss versions and therefore the test has been marked to be skipped.
Reason is that we handle unit and quantity files more strict and because of that, vss had to be patched (COVESA/vehicle_signal_specification#758). Old release are decided not to be patched
Points of interest / Questions / Code TODOs
arraysize
attribute. On purpose? 🆗 Not on purpose but we keep not exporting it for now until neededvalidate
what is it? 🆗 Clarified. In future to be discussed whether it should be part of the model when exporters explicitly mentioning it.--strict/--aborts name-style
behavior? 🆗 Type tree does not need to follow name style conventions. It would lead to weird property namesAdded
vspec export tree
)Todo
has
as valid bool prefixExtending instances implicitly
Take this config
Although there are only
Row1
andRow2
created the user wants to add another one implicitly.However that is some weird magic since it is unlogical to distinguish the behavior from what is displayed in the config: Adding something to all instances. Just because the name of
Row3
matches the instance expansions we should not rely on that imo.So, the outcome of the config as is would be in this implementation:
That is obviously not what the config intended but in my opinion the config is incorrect. When changing
instances
toRow[1,3]
we get the correct result:Validation improvements
Cross checking of
allowed-datatypes
Taking this example config:
Is detected as a misconfiguration since
numerics
does not exist but passes the current vss-tools model:Cross checking of
quantity
being definedPretty similar config but using a quantity that is not defined:
Is detected by tool but not by vss-tools:
Strict units yaml structure
Protecting against wrong structure in units.yaml and quantities.yaml such as using
description
instead ofdefinition
,units
instead ofunit
We actually have already violations in the current vss:
Cross checking
arraysize
todatatype
being an arrayExample config
Is detected by the tool (note that datatype is not an array) but vss-tools has no problem with it:
Similar: Also checks that
arraysize
matches the entries ofdefault
:Checking
default
againstdatatype
Example:
Is detected but is not currently:
Checking
allowed
entries matchingdatatype
Similar to the use case before, Config:
Note that
foo
is not an uint8 and should not be accepted.Is detected:
It also checks ranges e.g. -3 will violate uint8.
Checking
min/max
set together withallowed
Config:
Result:
Checking of
default
values againstallowed
Config:
Result:
Checking of
datatype
vsallowed_values
in specifiedunit
Config:
Result: