Autoformatting of Fortran source?

[awvwgk]

Should we have a formatting tool for fpm? It would be nice to not have to worry about any whitespace related discussions in a PR and instead have a CI tests running the formatter and reporting if changes are needed to match the style guide.

I tried lfortran fmt, fprettify and findent on fpm to check the impact, all have some drawbacks:

• lfortran fmt currently strips comments and use statements (using 0.8.1 from cf), fmt feature seems still WIP
• fprettify (0.3.6 from pypi) has some interesting understanding about intrinsic functions with an all or nothing setting for whitespace between the keyword and the parenthesis
• findent (3.1.7 from cf) just handles the indentation (exactly as it says on the tin), but does not indent continuation lines with a & character

So non of those would be able to preserve any of the current files as they are. I don't have a strong preference for either of those tools, as long as it takes away the burden to check for the style guide and whitespace conventions.

lfortran fmt and findent could be easily installed via conda in a CI workflow, fprettify can be installed by pip. Anything else I missed?

[certik]

I am happy to improve lfortran fmt if you would consider using it.

[awvwgk]

@certik lfortran fmt made the smallest diff not counting the missing comments and imports, so this would be great to have.

[certik]

@awvwgk ok, I am happy to fix those. I am tracking all the improvements to lfortran fmt here: https://gitlab.com/lfortran/lfortran/-/issues/212. Anything else?

How about empty lines (in a subroutine, as well as between subroutines) ---- how should those be handled: ignored / reformatted, or preserved?

[everythingfunctional]

I would definitely be in favor of using an automated tool to format the code. As you mention it could save a lot of time dealing with and arguing about style.

If there is an existing tool that works, I'd be in favor of switching to it now. Even if there are some idiosyncrasies about the format that we may not prefer, unless there's some deal-breaker, I'd rather just not have to worry about it anymore. And then yes, we should add a step in the CI to make sure running it on all the files doesn't change them.

[everythingfunctional]

My vote on empty lines would be to remove consecutive ones, so never have more than 1 blank line as a separator.

[certik]

Right now, lfortran fmt generates empty lines between subroutines and so on. This can be made configurable. But the question I have is if it should preserve empty lines put in by the user, such as in:

if (something) then
    i = 5

    j = 4
end if

Currently this will always be transformed into:

if (something) then
    i = 5
    j = 4
end if

Since clang-format preserves those, I think lfortran fmt should too.

Btw, if you are willing to use it, I'll work day and night to make it work.

[LKedward]

I'd prefer a fully-automated solution (over a pass/fail CI check) which commits formatting changes in the CI either during pull requests or on merge into master. See this article for an example.

The advantages of this approach are:

• no back-and-forth with the CI to get formatting checks to pass;
• local testing does not require contributors to download the formatting tool and maintain the same version between each other;
• keeps the PR process simple for new contributors and those not familiar with git/github;
• removes formatting workload from both contributor and reviewer;
• formatting changes are well-contained within specific formatting commits.

[everythingfunctional]

Good point @LKedward , I agree.

[certik]

The way it can work is that the formatting is applied (but not committed) before running CI tests, to ensure that the CI tests actually work after applying the formatting. Then after the PR is merged, the formatting is actually committed.

[everythingfunctional]

The way it can work is that the formatting is applied (but not committed) before running CI tests, to ensure that the CI tests actually work after applying the formatting. Then after the PR is merged, the formatting is actually committed.

I somewhat disagree. I think it should try and apply the formatting, and only if the tests fail (assuming they passed before) does it not actually commit. As a reviewer, I'd like to see the code that will actually be merged.

[awvwgk]

I got some ads for automated code style fixing some time ago solving exactly this kind of problem. I'll check if I find the app on the GH marketplace.

In short it does the following:

• runs a code formatter, (optionally) reports back the status to the PR
• in case there is a diff, it will be committed to a separate branch
• to apply the changes a PR against the authors branch is opened automatically
• the author can review and merge those changes or fix it locally instead

[certik]

Yes, I've been struggling with reviewing the final code --- being the author of the formater, I know that there is a possibility of a bug in it, and so I would also like to see the final code before it gets committed. @awvwgk's solution should work.

[awvwgk]

@certik you might be right, using lfortran fmt will probably run into plenty of bugs first and the fpm code base is too small to be a good test case. But since most of us are developing open source and/or proprietary Fortran code bases, we should easily get a few hundred thousand lines of Fortran source code together for cross checking the formatter. I'll volunteer my projects to check lfortran fmt for this purpose.

[awvwgk]

I got some ads for automated code style fixing some time ago solving exactly this kind of problem. I'll check if I find the app on the GH marketplace.

It is indeed a rather new project:

• source: https://github.com/restyled-io/restyled.io
• homepage: https://restyled.io/
• GH marketplace: https://github.com/marketplace/restyled-io

Of course Fortran is not under the supported languages, but it is written in Haskell, so we might be able to contribute fixes back in case we have to.

The alternative is to write it ourselves with GH actions. Using fprettify as a start is probably the best choice for now.

[MuellerSeb]

Since clang-format preserves those, I think lfortran fmt should too.

Also like black does it in Python: preserve 1 empty line, but more should be cut down to 1.