EAMxx clang-format Workflow

[mjschmidt271]

This page is intended to be for discussion or comments on formatting decisions related to the clang-format styling choices.

Topics

3-tab Indentation

• Seems pretty rad, but I think I’d prefer 2 spaces - @mjschmidt271

[bartgol]

+1 on 2 spaces. We have a few places with plenty of nesting levels, so keeping indentation small helps (I often have two terminals open, side-to-side). 2 spaces is the bare minimum that my eye can recognize.

[mahf708]

I prefer we remain as close as possible to defaults. I am interested in gathering what other projects do. Creating a list here:

• kokkos core: https://github.com/kokkos/kokkos/blob/develop/.clang-format
• xtensor: https://github.com/xtensor-stack/xtensor/blob/master/.clang-format
• pytorch: https://github.com/pytorch/pytorch/blob/main/.clang-format
• tensorflow: https://github.com/tensorflow/tensorflow/blob/master/.clang-format
• pybind11: https://github.com/pybind/pybind11/blob/master/.clang-format

[bartgol]

Side convo: what's the consensus on how the formatting should happen? I see two main options:

• users run it manually: when PR is ready, run formatter, make a formatting commit, push, and then integrate.
• automated: i don't want it to run for every push to the PR branch (don't want 2+ format commits per PR), but maybe it can run on master after a push? We'd have to ensure the same formatting commit is also merged into next though...

[mahf708]

Side convo: what's the consensus on how the formatting should happen? I see two main options:

• users run it manually: when PR is ready, run formatter, make a formatting commit, push, and then integrate.
• automated: i don't want it to run for every push to the PR branch (don't want 2+ format commits per PR), but maybe it can run on master after a push? We'd have to ensure the same formatting commit is also merged into next though...

users run it manually before they ask others to look at their work; ideally, it should be done in every commit, so every commit should have the correct formatting.

[philipwjones]

We have it set to do both. You can certainly run manually, but the typical workflow is to have it run as a pre-commit hook, so it notifies and modifies the code so you can do a git add with the fixed code before you actually commit. There are times when I don't have my develop environment set up so it doesn't catch on pre-commit which then requires an actual linting fix commit that we'd all like to avoid. Nothing is foolproof, but doing at the pre-commit stage definitely helps.

[mahf708]

We have it set to do both. You can certainly run manually, but the typical workflow is to have it run as a pre-commit hook, so it notifies and modifies the code so you can do a git add with the fixed code before you actually commit. There are times when I don't have my develop environment set up so it doesn't catch on pre-commit which then requires an actual linting fix commit that we'd all like to avoid. Nothing is foolproof, but doing at the pre-commit stage definitely helps.

Yes, I agree with the pre-commit idea and it will achieve the same thing as rebasing fixups in each commit cleanly (instead of dangling and endless "clang format" commits...)

[mjschmidt271]

Just to throw a wrench in things...

We'd discussed in a previous meetings that, at least at first, it may make sense to save the format commit until after reviews, since touching virtually every line makes it a little tricky to figure out the substantive changes.

However, the other option is the nuclear one where we format the entire repo in one go and then require formatting for PRs

[mahf708]

@mjschmidt271 what do you think about this?

We make the clang format action produce a nicely formatted patch and make that available as an artifact uploaded in the action?

manually, this could be achieved with something like:

clang-format aerocom_cld_util.hpp > tmp.xpp 
diff aerocom_cld_util.hpp tmp.xpp > format.diff
rm -rf tmp.xpp

you can test if it's working properly by

patch < format.diff aerocom_cld_util.hpp
clang-format aerocom_cld_util.hpp > tmp.xpp 
diff aerocom_cld_util.hpp tmp.xpp > format.diff
cat format.diff # should show nothing since formatting was applied in the patch <... line

To be clear, the goal: people can download the format.diff file and just apply it to their code locally if they don't want to run clang-format locally

[mjschmidt271]

That is a really cool idea! I honestly had never heard of/used patch before...

I'll work something up in the next week or so

[bartgol]

You could also let clang DO the modification, and then do `git diff > /some/path/patch.txt' (and upload the file as an artifact). As long as the action does not push anything back to the repo, you can modify whatever you want.

[bartgol]

Do you know what format setting makes this change?

-  EKAT_REQUIRE_MSG (n_a==ncols_fine or n_b==ncols_fine,
-      "Error! The input map seems incompatible with the remapper fine grid.\n"
-      " - map file: " + map_file + "\n"
-      " - map file n_a: " + std::to_string(n_a) + "\n"
-      " - map file n_b: " + std::to_string(n_b) + "\n"
-      " - fine grid ncols: " + std::to_string(ncols_fine) + "\n");
+  EKAT_REQUIRE_MSG(n_a == ncols_fine or n_b == ncols_fine,
+                   "Error! The input map seems incompatible with the remapper fine grid.\n"
+                   " - map file: " +
+                       map_file +
+                       "\n"
+                       " - map file n_a: " +
+                       std::to_string(n_a) +
+                       "\n"
+                       " - map file n_b: " +
+                       std::to_string(n_b) +
+                       "\n"
+                       " - fine grid ncols: " +
+                       std::to_string(ncols_fine) + "\n");

I personally find the latter unreadable (besides using more than 2x as many lines), so I would vote for disabling this "formatting" option.

[bartgol]

Ugh, I am not a fan of adding inline linter hints in our code (be that clang-format or pylint). I found a setting that does not break the string over multiple lines, but unfortunately it also removes any existing line break, making a single n>>1 chars long line...

[bartgol]

I wish clang would leave the code as is when a setting for that particular issue is not explicitly set. E.g., if user does not set the option for the alignment of reference symbols, just don't format that aspect... But it seems that the line break for stirng literals is either always enforced (giving that long suggestion I pasted) or never used (forcing the whole snippet above in one line).

[mjschmidt271]

@mahf708 was correct to doubt the bot. I had considered those options previously but that particular string seems to hit some frustrating corner cases, given that it uses multiline strings, successive strings, and binary operators.

@bartgol - I believe I found the same/similar behavior you mention that alters the line breaking--just doesn't alter it for the better.

My overall thought is directly aligned with @bartgol 's comment, and I would not want to see the code covered with inline formatting flags wherever people don't like what the formatter is asking for. Conversely, I also thing that this situation is one of a few rather rare ones where we can't universally configure clang-format to render it properly and the readability is greatly improved by user-defined formatting choices.

My proposal is to keep the current config behavior and use the formatting on/off flags judiciously. Then we leave it to PR reviewers to question appropriate usage. I can certainly vouch for the lack of reticence from EAMxx developers on usage of comments 🙂

[bartgol]

Yeah, the choice is between adding linter comments or having the CI always fail since we agree that the proposed change is not better, and won't be implemented.

I don't want to have CI actions always failing, as it pollutes all PRs with an X near them, which is not cute. I think I will bite the bullet and add clang-format specifiers. But maybe in a separate PR, where I also plan to modify some settings (will ping Mike and Jim offline to decide on settings).

[mahf708]

I personally don't mind that fugly format; if you look at some of my code in the repo, you will find insanely formatted stuff that's mainly due to clang-format. I honestly don't quite understand the logic behind why it insists on that format (seems like it's even hard to guess what it's trying to achieve?) ...