Skip to content
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

Fix usage of default #421

Closed
wants to merge 2 commits into from
Closed

Conversation

erikbosch
Copy link
Collaborator

This comes from a discussion on using default for structs. As of today we state in https://covesa.github.io/vehicle_signal_specification/rule_set/data_entry/data_types_struct/index.html#default-values that we do not support them, except for one special case.

This PR adds a check that matches that.

Side note: @UlfBj may propose a change to allow default for struct types. If so we will need to refactor the code that checks default to either:

  • Accept anything
  • Check that it has whatever format we specify it should have (json?)

f"--output {out} --types {types_file}"
)
env = os.environ.copy()
env["COLUMNS"] = "200"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think we should try to get away from checking stdout output due to the ugliness of the COLUMS thingy.
The good alternative I see is using --log-file and parsing that log file instead. That one does not have terminal width dependant breakpoints.
I have just created a PR that does that for all old tests: #424

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look at that one and then I can rebase/refatcor this one afterwards

Signed-off-by: Erik Jaegervall <[email protected]>
Signed-off-by: Erik Jaegervall <[email protected]>
@sschleemilch
Copy link
Collaborator

Might be not needed anymore (besides the test). Implemented default support for structs:
#425

@sschleemilch
Copy link
Collaborator

Might be not needed anymore (besides the test). Implemented default support for structs: #425

Added a test in #425

@erikbosch
Copy link
Collaborator Author

To be closed if #425 is merged

@erikbosch
Copy link
Collaborator Author

Closed as no longer needed

@erikbosch erikbosch closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants