Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
168 stars 40 forks source link

Expand namespacing with differentiating components #417

Closed GwnDaan closed 5 months ago

GwnDaan commented 5 months ago

I've stumbled upon constants being hidden inside declarations of classes for some time now. I find that it makes it less appealing to use them, because it adds unnecessary inheritance in the class wanting to use them. I know we have the can_constants.hpp header file and I would like to use it, however throwing everything on one pile can be less appealing as there are a few domain-specific constants that will bloat the isobus namespace universally.

So a few proposals, which I very much like feedback on:

If anyone has any thoughts on this, please let me know :)

edit: Also on mobile, the long class names make it harder to read code. Removing the prefixes will make them a lot shorter and therefore easier to read on smaller screens.

ad3154 commented 5 months ago

Oof, I dunno... besides the potential of obliterating all consuming applications compilation (which seems undesirable) the main benefit of shorter names has never been a particularly compelling one to me, as you lose the local context of which Client:: or whatever you are looking at when maintaining or reading the library's components. Plus, in ISO11783 there's lots of overlap between things like VT and TC and Implement messages... I guess I'd be happier trying to figure out better solutions to specific constants you're having pains with

GwnDaan commented 5 months ago

Okay, that's indeed undesirable for resolving a small issue. So what I would need now is the NULL_OBJECT_ID, which indeed is overlapping between TC / VT so differentiating in namespaces wouldn't have made much sense. Perhaps just adding it to the constants header file is fine?