Enforcing code style

[jewatkins]

@ikalash @mperego @bartgol @kliegeois @mcarlson801

It looks like clang-format does require clang. It probably needs to be a specific version of clang too in order to be consistent (I've seen some issues with using different versions).

Also, it looks like creating a git hook to enforce formatting would require everyone to have to install the git hook locally. So something like:

git clone Albany
cd Albany
source tools/install_git_hooks.sh

so it would still require some level of diligence.

[jewatkins]

Alternatively, if we do add a CI build for PRs (let's say with blake-serial-Albany-gcc-no-warn), we could check formatting too. That might be the easier approach.

[bartgol]

I'm all for enforcing code style. I think clang-format is a fairly common approach, and even if it requires a tiny bit of effort to install the compiler, it's probably worth it.

I suspect the clang version is somewhat irrelevant (so long as it can parse the code, meaning it must support the same c++ std that Albany uses).

The easiest way to do this could be to: 1) disable push to master (i.e., force to go through PR, even for small fixes), 2) have PR reviewers control that the code contains a clang-format as last commit (i.e., push a clang-format when ready to merge, then merge).

P.s.: sorry if I said stuff you already discussed during the telecon.

[jewatkins]

The easiest way to do this could be to: 1) disable push to master (i.e., force to go through PR, even for small fixes), 2) have PR reviewers control that the code contains a clang-format as last commit (i.e., push a clang-format when ready to merge, then merge).

Hmm that actually does sound much easier to implement quickly than the previous suggestions. But disabling push to master might be too restrictive (I'm thinking all the dashboard/test fixes). Even having a standard format script and being able to use it after a PR is finished would be a huge step forward.

[kliegeois]

I am currently looking into the CI workflow templates of GitHub if I can find anything that could help us.

[bartgol]

The easiest way to do this could be to: 1) disable push to master (i.e., force to go through PR, even for small fixes), 2) have PR reviewers control that the code contains a clang-format as last commit (i.e., push a clang-format when ready to merge, then merge).

Hmm that actually does sound much easier to implement quickly than the previous suggestions. But disabling push to master might be too restrictive (I'm thinking all the dashboard/test fixes). Even having a standard format script and being able to use it after a PR is finished would be a huge step forward.

Yeah, the protection of master is not really necessary. I was thinking that someone might push straight to master with bad formatting, but since we're a small group (and outsiders don't have the right to push, I think), then it's fine to leave master open.

[kliegeois]

The easiest way to do this could be to: 1) disable push to master (i.e., force to go through PR, even for small fixes), 2) have PR reviewers control that the code contains a clang-format as last commit (i.e., push a clang-format when ready to merge, then merge).

Hmm that actually does sound much easier to implement quickly than the previous suggestions. But disabling push to master might be too restrictive (I'm thinking all the dashboard/test fixes). Even having a standard format script and being able to use it after a PR is finished would be a huge step forward.

Yeah, the protection of master is not really necessary. I was thinking that someone might push straight to master with bad formatting, but since we're a small group (and outsiders don't have the right to push, I think), then it's fine to leave master open.

I confirm for the outsiders.

[ikalash]

@lxmota : do you use a clang format script to format Albany LCM source code? Is it the generic one available online, or you modified it somehow? Also, I assume a clang compiler is required to use the script?

[lxmota]

@ikalash I use a template that is here: https://github.com/SNLComputation/LCM/blob/master/.clang-format, which you can modify to suit the required coding style.

At least on Fedora, clang-format does not require the compiler front end, but it uses a lot of the compiler's infrastructure like LLVM and Clang libraries, so you might as well install the full compiler. It can be installed by dnf install clang-format.

Also, there is a git-clang-format command that it will run clang-format on un-committed changes in a repo, after which the changes can be committed. It's very handy. On Fedora you get that with dnf install git-clang-format.