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 typos in proto files #774

Merged

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Feb 12, 2024

Reference to a related issue in the repository

fixes #773

Add a description

There were a lot of typos in the proto files. I fixed the ones I found.

Two things I am unsure of and therefore did not fix yet:

  1. Unfortunately, I found one spelling mistake in an enum in osi_object.proto: It should by TYPE_GLASS and not TYPE_GLAS. Furthermore, sometimes British English is used in enums, e.g. COLOR_GREY. I suppose fixing it would break backwards compatibility?
    -> Enum names shall not be changed in a minor release
  2. In osi_lane.proto the term antecessor is used in multiple places. Is this really the correct term? I am more used to ancestor in this context.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

@ClemensLinnhoff ClemensLinnhoff linked an issue Feb 12, 2024 that may be closed by this pull request
@ClemensLinnhoff
Copy link
Contributor Author

Once the two mentioned issues are discussed, we can remove the draft status and mark as ready for CCB.

@PhRosenberger
Copy link
Contributor

As I also struggle with both questions, who else could support here?
@ThomasNaderBMW @pmai @jdsika @thempen

@jdsika
Copy link
Contributor

jdsika commented Feb 13, 2024

Ancestor would be a term that I see for human relations like "My ancestors were brave fighters in the galaktic resistance" whereas "antecessor" is often used for e.g. "the Ferrari 5 series is the antecessor of the Ferrari 567". The usage in combination with objects would make it more suitable in my option for our digital objects like lanes but ... I am not a native speaker. Most important in this case would be "does everyone understand what is meant by it"?
Regarding the "GLASS" and "GLAS" ... It would in fact break compatibility... Let's bring it up for a vote. I think we have fixed blunt mistakes in the past even without a major version change?

@jdsika jdsika added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Feb 13, 2024
@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Feb 13, 2024

The cambridge dictionary does not even list antecessor: https://dictionary.cambridge.org/spellcheck/english/?q=antecessor

Wiktionary says it is a rare synonym of ancestor: https://en.wiktionary.org/wiki/antecessor

I personally have never heard of it until now, but I am also not a native speaker. "Predecessor" would be another option, I am familiar with.

@ClemensLinnhoff
Copy link
Contributor Author

As for the typo of MATERIAL_GLASS, I am in favor of fixing it. I suspect, that this enum field is not that commonly used anyways. So the backwards compatibility issues will most likely be limited.

@ClemensLinnhoff
Copy link
Contributor Author

Furthermore, we have inconsistencies with British and US englisch. Mostly, US is used, but in some cases, unfortunately not only in comments, we have British, e.g. COLOR_GREY instead of COLOR_GRAY.

@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Feb 14, 2024

What does FCW stand for here? It is never explained.

@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Feb 14, 2024

I now added a spellcheck pipeline and fixed a lot more spelling mistakes.
You can find the current result here.

I had to add quite a lot of exceptions, because of abbreviations and also a lot of German words.

@PhRosenberger
Copy link
Contributor

What does FCW stand for here? It is never explained.

I guess FCW stands for Forward Collision Warning in this case.

Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

Otherwise looks good to me. Spellchecker potentially needs more fine-tuning, and we cannot change enum names per-se in a minor release. We might be able to add alias names, however would first have to check protobuf behavior.

osi_sensorview.proto Outdated Show resolved Hide resolved
osi_environment.proto Outdated Show resolved Hide resolved
@ClemensLinnhoff
Copy link
Contributor Author

Otherwise looks good to me. Spellchecker potentially needs more fine-tuning, and we cannot change enum names per-se in a minor release. We might be able to add alias names, however would first have to check protobuf behavior.

Thank you for the review, I documented the decision about leaving the enums in the PR description. I fixed the spelling mistakes in the comments belonging to the enums, but not the enums themselves.

From my point of view now the only open point is the use of antecessor vs. ancestor.

@pmai
Copy link
Contributor

pmai commented Feb 26, 2024

CCB 2024-02-26: As antecessor is used also in e.g. the LanePairing field antecessor_lane_id a change here would require a major release. Therefore antecessor should stay as the term in the documentation as well. @pmai will investigate whether adding proper spellings for TYPE_GLAS and COLOR_GREY as aliases would work. If yes, they can be added. The existing enums have to stay in in any case to remain backward-compatible. After the above changes and finalization this PR is ready for merge.

@ClemensLinnhoff ClemensLinnhoff marked this pull request as ready for review February 26, 2024 10:32
@ClemensLinnhoff
Copy link
Contributor Author

I added antecessor to the custom word list. Now the spell check passes.

ClemensLinnhoff and others added 12 commits February 26, 2024 12:39
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Co-authored-by: Pierre R. Mai <[email protected]>
Signed-off-by: Clemens Linnhoff <[email protected]>
Co-authored-by: Pierre R. Mai <[email protected]>
Signed-off-by: Clemens Linnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
Signed-off-by: ClemensLinnhoff <[email protected]>
@pmai pmai force-pushed the 773-typo-in-osi3trafficactionendactionsaction-documentation branch from c41d856 to 739f9a9 Compare February 26, 2024 11:40
@pmai pmai force-pushed the 773-typo-in-osi3trafficactionendactionsaction-documentation branch from 739f9a9 to bbf5d34 Compare February 26, 2024 11:49
@pmai pmai force-pushed the 773-typo-in-osi3trafficactionendactionsaction-documentation branch from bbf5d34 to 0f6d424 Compare February 26, 2024 11:54
@pmai
Copy link
Contributor

pmai commented Feb 26, 2024

I have added aliases for the correct spellings of COLOR_GRAY and MATERIAL_GLASS, retaining the old COLOR_GREY and MATERIAL_GLAS enums with deprectation warnings. This should work for proto2/proto3, and the documentation generation; however please check whether this change breaks anything else...

@pmai pmai merged commit 659a95c into master Mar 8, 2024
8 checks passed
@pmai pmai deleted the 773-typo-in-osi3trafficactionendactionsaction-documentation branch March 8, 2024 10:47
@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Apr 4, 2024
@pmai pmai added this to the V3.7.0 milestone Apr 4, 2024
Copy link
Contributor

@jdsika jdsika left a comment

Choose a reason for hiding this comment

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

Possible changes during review v3.7.0

//
// ColorGrey defines a greyscale.
// ColorGrey defines a grayscale.
//
message ColorGrey
Copy link
Contributor

Choose a reason for hiding this comment

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

@pmai do we need an Alias here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, let me guess: Alias only works for ENUMS and not signals? Should we add a TODO here? Does it bother?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typo in osi3::TrafficAction::EndActionsAction documentation
4 participants