machines-in-motion / dynamic_graph_manager

BSD 3-Clause "New" or "Revised" License
4 stars 4 forks source link

File name convention #12

Closed MaximilienNaveau closed 4 years ago

MaximilienNaveau commented 4 years ago

Problem

The file name convention is wrong for the headers.

Potential solution

use the .hpp extension instead of the hh

lberrymage commented 4 years ago

The file extensions used are not wrong, per se. Many header extensions are acceptable for C++, including ".h", ".hh", ".hpp", ".hxx", and ".h++". Which to use is up to a project maintainer's personal preference, and I can see no indication of it being ".hpp" for this particular project. ".hh" is obviously C++ to anyone looking, so I don't understand why you want to change it.

A notable problem presented by a change like the one you're proposing is an API incompatibility. People currently using the library who update it will be treated with missing #include's everywhere they use the library since the header names have changed. This is not inherently bad, but must be reserved for a major version change as opposed to a minor one. Breaking changes need to be treated with care.

Since you're a collaborator, you probably know the will of the maintainer better than I do. I would be happy to add a pull request if they approve. Even so, I wouldn't try to alter the convention currently used by the project for reasons mentioned above. No user will be surprised (or care) if the file extension is ".hh", but they will if a breaking API change (like the one you're proposing) is slipped into a minor update. Just something to note.

gazarahmad commented 4 years ago

i agree that this is an important point and changing extensions should not be done lightly. therefore i urge @MaximilienNaveau to reconsider his plans. I suggest we set a video call to discuss this issue.

lberrymage commented 4 years ago

@gazarahmad I don't think a video call is necessary for something this trivial, nor convenient for external contributors. This discussion section serves its purpose plenty well.

gazarahmad commented 4 years ago

agreed, we can pursue this discussion here for now, but in the future we may want to have video meetings on a regular basis. nevertheless, i would like to reiterate my concern over the possible ramifications of such a change. in addition i would like to advise @MaximilienNaveau to exercise more restraint in the future and to report any progress on this issue directly to us.

vincentberenz commented 4 years ago

agreed on the video meeting. This would give us a chance to explain to @MaximilienNaveau the naming conventions regarding header files.

gazarahmad commented 4 years ago

perfect, so we should schedule a meeting ASAP. i would suggest that we also include @sheim as his expertise may be valuable in this matter. i would suggest the following meeting agenda, please feel free to amend it as you see fit:

  1. how do we make sure that such transgressions, be it due to malicious intent or plain ignorance, do not happen again? what can we use as carrots and sticks?
  2. how do we resolve the concrete issue at hand? my suggestion: for each header, we provide a copy for each extension (".h", ".hh", ".hpp", ".hxx", and ".h++"). this way we do not have to commit to one extension, which would be severely limiting, instead we can use all of them at the same time! in addition, this enables the users to use any extension of their choice. to absorb the slight overhead of such a decision @MaximilienNaveau could implement scripts which will be triggered at compilation time.
  3. we should form a committee to evaluate the feasibility of extending this approach also to source files, such that we can use simultaneously all extensions (".cc", ".cpp", ".c++" and ".cxx").

please let me know in case you disagree with any of the above.

lberrymage commented 4 years ago

I apologize @gazarahmad - I wasn't originally under the impression that you were part of the development team (or equivalent). Your GitHub profile indicates you joined GitHub today.

gazarahmad commented 4 years ago

No problem, thanks for pointing out the issue!