Perform simple format checks for new PR's

[ischoegl]

Abstract

Perform some simple format checks for new PR's as part of GitHub Actions to ensure new code complies with coding style recommendations.

Motivation

This feature request is based on a remark by @bryanwweber on Cantera/cantera#1007, which pertains to an update to the recommended line lengths in Cantera's coding style guidelines:

It'd be really nice to have an automated linter that could check these things for us, but I haven't found one that we can sufficiently configure and which will ignore lines that aren't changed in a particular PR.

Possible Solutions

As an alternative to linters, it may be relatively straight-forward to implement simple compliance checks. One example would be to use shell commands to print newly added lines that are longer than recommended:

git diff main... | awk '/^+[^+]/ && length($0)>89 { print substr($0,2); }'

where git diff main... shows code changes since the last common commit of the PR branch and main, and the regex /^+[^+]/ is used to locate lines that were added (which for the diff output start with a single +). Non-zero output can be used to trigger pipeline failures.

Similar checks can likely be implemented to locate trailing whitespaces, tabs, instances of doublereal etc.

References

Cantera/cantera#1007

[ischoegl]

Reopening this due to a comment on code style issues by @speth in #93

I find that even with the limited style rules that we do try to enforce, it's challenging for comments about these sorts of issues not to drown out more substantive review questions about implementation and correctness, and depending on how authors choose to address such style issues, can lead to a really bloated commit history (especially if one uses the option to directly accept suggestions made in a review). I think this is already a problem, so maybe we should have a separate discussion on the topic.

Getting failing GitHub 'linting' actions may automate this, but as @bryanwweber mentioned elsewhere, configuring linters may not be simple. A final solution (if ever pursued) will likely look very different from my initial suggestion above.

PS: quick search result ... in a nutshell: projects exist, but long-term maintenance is an issue

[speth]

For C++, LLVM provides clang-tidy, which focuses on actual usage issues, and clang-format, which focuses on formatting and whitespace. Both include scripts to limit their warnings to only include lines indicated in a diff. The tidy tool can be run with something like:

git diff -U0 HEAD^..HEAD | clang-tidy-diff -p1 -checks=readability-*,clang-analyzer-* -- -I./include -isystem./ext/googletest/googletest/include -x c++

which will look only at changes in the last git commit. There's a long list of checks that can be enabled/disabled depending on what we would want to check for, and similarly for clang-format-diff, a lot of configuration on what formatting style to check for. clang-format can also be configured to apply the specified formatting as well.

One possibility for avoiding some issues before they even get to the PR stage would be to have configurations for using these tools as Git pre-commit hooks. Of course, I'm not sure how we reliably get contributors to enable these hooks.

[ischoegl]

:+1: ... have never used LLVM, but it sounds good. Required pre-commit hooks may be a high entrance bar for new contributors. As long as the error logs are clear, fixing issues for a failing test may be much easier. Python is another (important) matter of course.

[bryanwweber]

Of course, I'm not sure how we reliably get contributors to enable these hooks.

If we have a CI job that runs before any builds run, which checks that the hooks are satisfied and leaves a helpful message if not, that'd be one way. 🎵 If I had a million hours... 🎶

[speth]

I just discovered git rebase --whitespace=fix main and it's amazing. It automatically fixes most of the trailing whitespace issues retroactively. There's some configurability via the core.whitespace git config setting, but I haven't fully explored that yet. The one thing I wasn't able to get it to do so far was transform CRLF to LF line endings.

[bryanwweber]

There's a setting core.autocrlf that controls the line endings, in part. I remember struggling with that when I had a Windows box...

[speth]

Yes, I'm aware of that option, and people should set that appropriately on their Windows systems. My interest is more in how to fix the issue after the fact as part of a rebase operation, to avoid introducing a commit that looks like a whole-file replacement. The only way I know is via git filter-branch, but that's kind of a nuclear sledgehammer (and just about as dangerous).

[ischoegl]

Withdrawing - this looks too much like bike-shedding to me.

[speth]

A few checks of this sort have been implemented in Cantera/cantera#1566.