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

Renaming "allowed" literals according to guidelines #404

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

erikbosch
Copy link
Collaborator

@erikbosch erikbosch commented Jan 26, 2022

This is an attempt to harmonize the use of literals for "allowed" values in the VSS standard, so that they as default use only uppercase, number and underscore. That has in this PR been consistently changed, but there might be some cases where it can be discussed if the recommendation shall be followed or not.

In most cases the change is straight forward, but for some a bit more advanced "translation" has been proposed, e.g. replacing kWh/100mi with KILOWATT_HOURS_PER_100_MILES.

This PR has not analyzed if the values/signals actually make sense, i.e. no values have been added/removed, the semantic meaning of each is supposed to be the same after the rename. Also UNKNOWN is kept as is, it need to be discussed on signal by signal basis whether it makes sense to keep it or not.

Notes:

Private-branch (CARFIT) not updated - private branch so not covered by recommendations. Branches reflecting other standards/solutions might also be a typical case where it might be a reason not to follow the recommendation. If e.g. CARFIT use abc, then it might be less useful to require that it shall be ABC in VSS.

Copy link
Collaborator

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

Looks good.
Found some instances where we might need to discuss semantics, but as you mentioned, let's first just convert the format

gunnarx
gunnarx previously requested changes Feb 1, 2022
spec/Cabin/Infotainment.vspec Outdated Show resolved Hide resolved
@gunnarx gunnarx dismissed their stale review February 1, 2022 18:38

Approved today, conditional on change

@erikbosch
Copy link
Collaborator Author

@gunnarx - now PR is updated

@erikbosch
Copy link
Collaborator Author

Maybe we shall wait a little with merging this one. I noticed that our tooling do some unexpected conversion of string literals not quoted, well at least unexpected for me.

See COVESA/vss-tools#143

FYI: @gunnarx @danielwilms @SebastianSchildt

@erikbosch
Copy link
Collaborator Author

Meeting notes:

  • Keep PR on hold until linked issue have been investigated

Also changing guidelines to recommend quotation marks.
Background: Otherwise literals like ON, OFF, FALSE might
be transformed by tooling.

Notes:

Private-branch (CARFIT) not updated - private branch so
not covered by recommendations

Signed-off-by: Erik Jaegervall <erik.jaegervall@se.bosch.com>
@danielwilms
Copy link
Collaborator

meeting approved 15/02/22

@danielwilms danielwilms merged commit 2b24d0d into COVESA:master Feb 15, 2022
@erikbosch erikbosch deleted the erik_allowed branch June 20, 2022 08:41
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.

4 participants