Zanduino / BME680

Arduino Library to access the Bosch BME680 - temperature, pressure, humidity and gas sensor
GNU General Public License v3.0
37 stars 10 forks source link

SensorTypes enum in Zanshin_BME860 uses camelCase instead of SCREAMING_SNAKE_CASE #37

Closed raphael-bmec-co closed 2 years ago

raphael-bmec-co commented 2 years ago

This is causing naming conflict with any classes: TemperatureSensor, HumiditySensor, PressureSensor, GasSensor

SV-Zanshin commented 2 years ago

While this is not a bug and the google style guide that I use states that mixed case should be used (see https://google.github.io/styleguide/cppguide.html#Enumerator_Names) I think that the scope of the enumerated class can be moved into the class definition so that those values are not visible outside of the class. I'm away from my system right now, but perhaps you can move that enum into the class definition to check this.

The reason I hesitate to change this is because any change here might invalidate existing code.

raphael-bmec-co commented 2 years ago

Thanks for the feedback - funny that google changed it for essentially same reason :).

If I'm reading it correctly, they do add a "k" to the beginning of each constant/enum because they also use CamelCase for their classes.

Agree, this isn't really a bug and I appreciate the resistance to changing it. I had a quick look at the library and a bunch of the examples use those enums so moving it into the class would probably be doable but would be a breaking change for other peoples code.

Let me know your thoughts but I guess we should deal with the name clash on our end?

SV-Zanshin commented 2 years ago

What I could do is define synonyms for these enumerated values ( TEMPERATURE_SENSOR, HUMIDITY_SENSOR, PRESSURE_SENSOR and GAS_SENSOR) and change the documentation as well. And the old values will be marked as "deprecated" and at some version in the future they will be removed. While no solution to your current problem, it would clean up existing code for the future. What do you think?

Alain2019 commented 2 years ago

Don't do that, you' re just making more problems. Bring the enum's inside the class or use a namespace.

A namespace can be done almost without breaking existing code.

SV-Zanshin commented 2 years ago

Since the 4 enumerated values are used as parameters to various library function calls I don't see how encapsulating it within the class or in a namespace would work. So perhaps we might just leave it as is...

Alain2019 commented 2 years ago

Adding a "using namespace" makes the namespace "transparant". Just like often with the "std" namespace, at least in old "pre" namespaces code.

SV-Zanshin commented 2 years ago

Yes, but it still won't allow using "TemperatureSensor" as both an enum and a class in the same program, will it?

Alain2019 commented 2 years ago

Then the user has the option to use a "using namespace" clause or not.
For new code I advice not to use a using namespace, but explicitly add the namespace to the enums.

for example: use "std::string" instead of using namespace std; and using string

raphael-bmec-co commented 2 years ago

This would solve our problem and is how we do things internally. But again, I appreciate it would be a breaking change in that other users would need to make a change to their code, albeit a simple one, so I'll leave it to you to implement as discussed or close the issue without change.

Thanks all for the prompt engagement.

SV-Zanshin commented 2 years ago

Unfortunately this can't be fixed without breaking the current implementation