search

lofar-astron / DP3

DP3: streaming processing pipeline for radio interferometric data
GNU General Public License v3.0
15 stars 10 forks source link

Merge and simplify formatting scripts. #278

Closed mnijhuis-tos closed 4 years ago

mnijhuis-tos commented 4 years ago

This change significantly reduces the formatting script size while still maintaining full functionality.

jmmaljaars commented 4 years ago

I can see the point why you want to simplify this - for the specific dp3 case at least. Nevertheless I wonder whether we actually would like to do this, since:

  1. originally, the run-clang-format.sh script could be added to the pre-commit hook to do the reformatting when committing. In your implementation, you actually would do the format check if you add the script to the pre-commit hook. I wouldn't be in favor of that.
  2. You restricted the applicability to *.h and *.cc files only, and deleted the option to exclude certain directories from being reformatted. That's probably ok for the DP3 repo, but the formatting scripts are intended to be re-used across multiple repositories, e.g. wsclean (with *.h and *.cpp files) and everybeam - with a couple of externals that you definitely don't want to reformat.

I can live with your edits regarding 2, but I wouldn't opt for your choices regarding 1, i.e. I am still in favor of splitting it up in a run and a check (of course, it's perfectly fine with me if you do the check entirely in the CI/CD yml file)