BlueBrain / hpc-coding-conventions

Apache License 2.0
8 stars 5 forks source link

Make the configuration files visible #77

Closed alkino closed 3 years ago

alkino commented 4 years ago

The configuration file often have to start with a . (dot) to be hidden for the developer. The purpose of this project is to modify them and copy them in the right place, so we can make them visible.

alkino commented 3 years ago

Thank you for your patch @alkino But I must confess that I only agree with the change that makes ClangFormat configurations visible files.

* I would like to keep the change you removed in `cpp/cmake-format.yaml` that demonstrates developers how to declare to cmake-format the prototype of new functions. Why did you removed it? You thought it was dead code? Maybe we should add a comment.

Hm, if I understand correctly it is the file that is used for formatting cmake files. If so, yes, it is dead code, because we have no function called FOO BAR BAZ This part is in basic documentation of cmake-format, not sure it gives something to add it here: https://github.com/cheshirekow/cmake_format#custom-commands

* `PROJECT_SOURCE_DIR/.clang-format` is a dependency of other tasks, so it is important that its modification date is updated only when necessary. It explains why the previous `do_merge_yaml` was only updating the file when the new content is different. I would like to keep it that way.

Ok, I can put this functionality back.

* I don't understand the point of the new `--output` option. Do you want to write the result to standard output when `--output` is not passed? Because the proposed change is not doing that.
  ```
  Help on function dump in module yaml:
  dump(data, stream=None, Dumper=<class 'yaml.dumper.Dumper'>, **kwds)
      Serialize a Python object into a YAML stream.
      If stream is None, return the produced string instead.
  ```

  IMO this `--output` is not adding any information. If you want to dump to standard output, you can pass /dev/stdout as last argument. It is like the CLI of the `cp` or `mv` commands where you don't specify `--output` before the last argument.

--output is mandatory by the parser. The purpose was to make something more understandable in the cmake file.

${PYTHON_EXECUTABLE} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/cpplib.py" merge-yaml
"${CMAKE_CURRENT_SOURCE_DIR}/.cmake-format.yaml"
"${PROJECT_SOURCE_DIR}/.cmake-format.changes.yaml"
"${PROJECT_SOURCE_DIR}/.cmake-format.yaml")

Here it is no so obvious this is the cp or mv semantic.

* the argument parser in the `main` function is designed such that it is easy to add a new command-line action to `cpplib.py` that might be required some time in the future. I would like to keep this genericity instead of limiting the argument parser to only merging YAML.

I think it was a lot of boilerplat for maybe one day a new option.

tristan0x commented 3 years ago

Thank you for your patch @alkino But I must confess that I only agree with the change that makes ClangFormat configurations visible files.

* I would like to keep the change you removed in `cpp/cmake-format.yaml` that demonstrates developers how to declare to cmake-format the prototype of new functions. Why did you removed it? You thought it was dead code? Maybe we should add a comment.

Hm, if I understand correctly it is the file that is used for formatting cmake files. If so, yes, it is dead code, because we have no function called FOO BAR BAZ This part is in basic documentation of cmake-format, not sure it gives something to add it here: https://github.com/cheshirekow/cmake_format#custom-commands

Fair enough. I am ok to remove the section in the .cmake-format, I will add some documentation about it (see #78)

tristan0x commented 3 years ago

--output is mandatory by the parser. The purpose was to make something more understandable in the cmake file.

          ${PYTHON_EXECUTABLE} "${CMAKE_CURRENT_SOURCE_DIR}/cmake/cpplib.py" merge-yaml
          "${CMAKE_CURRENT_SOURCE_DIR}/.cmake-format.yaml"
          "${PROJECT_SOURCE_DIR}/.cmake-format.changes.yaml"
          "${PROJECT_SOURCE_DIR}/.cmake-format.yaml")

Here it is no so obvious this is the cp or mv semantic.

It is obvious to me but I am ok to meet you halfway: I am not against adding an --output option if:

tristan0x commented 3 years ago
* the argument parser in the `main` function is designed such that it is easy to add a new command-line action to `cpplib.py` that might be required some time in the future. I would like to keep this genericity instead of limiting the argument parser to only merging YAML.

I think it was a lot of boilerplat for maybe one day a new option.

Please revert this change, it makes adding a new action a pain (revert it basically).

alkino commented 3 years ago

I keep only the part on which every people agree.

The end, in a new patch.