owl-project / NVISII

Apache License 2.0
327 stars 28 forks source link

Namespace visii:: missing #86

Closed lpisha closed 3 years ago

lpisha commented 3 years ago

ViSII functions are in the global namespace, and they do not follow C naming conventions either (e.g. ViSII_initialize, etc.). This is bad practice in C++ and risks causing clashes with user application code.

natevm commented 3 years ago

First, since ViSII is also a python API, some compromises need to be made to stay roughly compliant with both languages. We do not aim to conform 100% to either language styles, since it’s just not possible to make both camps happy. E.g. Python standard is to require lower_underscores for parameter names, which can be set using kwargs, and are taken directly from the C++ header file names. If I were to use C++ compliant lowerCamelCase, that would have a significantly negative impact on the look of the Python API. (I also have no ability to transform their names from LCC to underscores like I can the function names, at least as of SWIG 4).

That being said, I do intend to add namespaces for all these functions, but it just keeps getting kicked down the road unfortunately. So for example, to initialize, the planned API is visii::initialize(...); which would roughly match the python visii.initialize(...) pattern. (Essentially, python’s package syntax can be made to roughly match C++’s namespace syntax). We would then only have the “visii” namespace (with no nested namespaces, since that would cause languages to unnecessarily deviate)

And just to be clear, there is no plan to support this “ViSII_initialize()” pattern ( the current api is just “initialize()” ). 🙂

lpisha commented 3 years ago

Just to be clear, I meant the comment as "either ViSII API functions should be within namespace visii, or they should be with the C naming convention of LibraryName_function, the fact that they're neither is bad practice". I don't have particular opinions on the case issue. Glad to see the namespace is planned.

natevm commented 3 years ago

Ah, sorry, I didn’t realize that was a C convention. I should be able to work on this soon.

natevm commented 3 years ago

Just added this to development. Will hopefully trickle into master soon.