Climatemachine.jl: Formatting as separate commit

[kpamnany]

Description

We are currently asking for JuliaFormatter changes to be a separate commit in a PR. I think this should be removed.

Additional context

We want to keep a clean commit history on master, which is the whole point of asking people to squash. Formatting commits are redundant to the development process and will obscure the history.

For CLIMA Developers

• [ ] This feature can be added (if it cannot be, explain why in a comment below -- lack of technical expertise, not relevant to the scope of this project, too ambitious)
• [ ] There is a timeline for when this feature can be implemented
• [ ] The feature has been (or will be) implemented (Please link the PR)

[kpamnany]

Cc: @simonbyrne for thoughts.

[leios]

Right, this is inconsistent. It was requested by @lcw to have the formatting applied as a separate commit, but I am failing to remember the exact justification.

I thought the workflow we were going for was:

1. Apply changes
2. Review changes
3. Apply formatter
4. Squash relevant commits and merge

Separating out the application of the formatter until after the review step allows people to play with the code a bit better while under review.

[thomasgibson]

I get the importance of having a clean commit history. Really, I do. But this may start becoming more cumbersome as new features and additions are added in PRs. Especially since the CI now appears to be checking for correctly formatted code.

Why not just let individuals format PR by PR (as suggested by @leios)? Or better yet, why don't we just format all of CLIMA in one single PR? Get the pain over with in one go.

[simonbyrne]

Or better yet, why don't we just format all of CLIMA in one single PR? Get the pain over with in one go.

This might be the best option: I was originally reluctant as it would break all open PRs, but I think if users also apply the formatter to their PR, they should end up with a nice diff.

[lcw]

I am for one commit that formats all of the code.

[lcw]

We can then run the formatter on each commit of open PRs to sync up. One could probably write a shell script that does this and merges the changes into each commit for a given range of git commits.

[simonbyrne]

As long as users remember to git pull

[lcw]

Do we know if the Formatter is stable? In the since if we run it twice do we get the same results? We used to use GNU indent [[1]] and there were corner cases where it did not converge to a fixed point.

[kpamnany]

I think the Github check is relying on the stability of the formatter, so if it is inconsistent, we will have more problems!

[leios]

Here's the diff when applying julia formatter to the current codebase: https://github.com/climate-machine/CLIMA/commit/d5830f67ad15295c2bce866a93cc523b9bdac5d9

I think a benefit of applying incremental changes is that we can ensure the formatter is working as intended with every incoming PR; however, I have no problem just applying everything at once if people are happy with the changes.

[jkozdon]

It will be a lot easier to see what a given PR does if there are not a bunch of formatting changes on top of the functionality changes to parse out, so I am all for applying and just getting it done.

Also, as soon as a few PRs come through we'll feel the pain of the formatter, so might as well just rip the bandaid off and get it over with.

[thomasgibson]

I get a different diff: 0bb6bfaaab3582a6d5395b14f417a0929ac9b1c5 but I think it's because I am formatting _every_ julia file (want to format tests too?). Also I applied the formatter a few times until all files would no longer update.

[lcw]

I get a different diff: 0bb6bfa but I think it's because I am formatting _every_ julia file (want to format tests too?). Also I applied the formatter a few times until all files would no longer update.

So it took a couple of formatting runs to reach a fixed point?

[leios]

I think I was just dumb and missed some files. I ran it again and got a similar number of lines changed: https://github.com/climate-machine/CLIMA/commit/d5830f67ad15295c2bce866a93cc523b9bdac5d9; however, there still seems to be a substantial diff between the two branches?

@thomasgibson how did you apply the formatter to every file? I just applied the formatter to find . -print

Running it multiple times does not seem to substantially the code in my fork (there are 3 files that are exceptions)

[thomasgibson]

I get a different diff: 0bb6bfa but I think it's because I am formatting _every_ julia file (want to format tests too?). Also I applied the formatter a few times until all files would no longer update.

So it took a couple of formatting runs to reach a fixed point?

Yes, I ran the formatter a total of 3 times. I got no more updates after the 2nd pass.

@leios I changed .dev/format.jl: I use readlines('git ls-files') which may not be the best option. But I wanted to try to make sure the formatter is applied to all .jl files in the repo.

[thomasgibson]

Running it multiple times does not seem to substantially the code in my fork (there are 3 files that are exceptions)

@leios Are these files in tests/DGmethods_old? I remember seeing three test files requiring an additional pass.

[leios]

Caught those. I was being dumb and accidentally pushed my changes to the wrong fork. We can generate the same diff off of master.

[simonbyrne]

It took 2 passes on master for me to get a fixed point: see https://github.com/climate-machine/CLIMA/tree/sb/format-fixed-point

The second pass had some problems with @sprintf: https://github.com/climate-machine/CLIMA/commit/7a3383753bcdeb5aab8f330faae98db52d706aea

[leios]

Is this because of a double macro call? I actually don't understand this splitting of lines at all.

[simonbyrne]

Okay, I've been trying to update #733 where there has been a mix of files that have been updated, and some that have not, and honestly it is a pain. I'm all for ripping the bandaid off.

[simonbyrne]

I've updated #536 to update all files in place.

[jkozdon]

What's the status on this?

[blallen]

What's the status on this?