OpenSimulationInterface / open-simulation-interface

A generic interface for the environmental perception of automated driving functions in virtual scenarios.
Other
272 stars 127 forks source link

Fix typos in proto files #774

Closed ClemensLinnhoff closed 8 months ago

ClemensLinnhoff commented 9 months ago

#### 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 commented 9 months ago

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

PhRosenberger commented 9 months ago

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

jdsika commented 9 months ago

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?

ClemensLinnhoff commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

ClemensLinnhoff commented 9 months ago

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 commented 9 months ago

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

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

ClemensLinnhoff commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

pmai commented 9 months ago

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...