fpm Autoformatting of Fortran source?

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?

Sep 30 '20 17:09 awvwgk

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

Sep 30 '20 19:09 certik

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

Sep 30 '20 20:09 awvwgk

@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?

Sep 30 '20 22:09 certik

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.

Sep 30 '20 22:09 everythingfunctional

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

Sep 30 '20 22:09 everythingfunctional

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.

Sep 30 '20 22:09 certik

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.

Oct 01 '20 08:10 LKedward

Good point @LKedward , I agree.

Oct 01 '20 15:10 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.

Oct 01 '20 15:10 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.

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.

Oct 01 '20 16:10 everythingfunctional

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

Oct 01 '20 16:10 awvwgk

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.

Oct 01 '20 17:10 certik

@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.

Oct 01 '20 18:10 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.

Oct 02 '20 08:10 awvwgk

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.

Oct 05 '20 10:10 MuellerSeb

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

Good idea. This can be the default, and we can make this configurable.

Oct 05 '20 12:10 certik

A few more linting tools:

A short evaluation of the tools is given in the following blog post:

Screening the coding style of Large Fortran HPC Codes

Ultimately, the authors (@dauptaia) decided to develop a new Fortran linter flint (https://pypi.org/project/flinter/) which also has some cool features like calculating a code score and display a circle packing of the source code lines colored according to conformance with the linting rules.

Mar 18 '21 23:03 ivan-pi

We should reconsider a consistent indentation style in fpm. Any preference for a formatter which we can apply on the whole code base?

I think lfortran fmt has improved quite a lot since we originally opened this issue (last time I tried it was still losing comments on certain statements).

Jan 06 '23 22:01 awvwgk

We haven't been focusing on the formatting. It is not difficult to resolve the issues, but I don't want to focus on it now, I think it is more important to compile fpm. Is anybody interested in helping out? I am happy to help you get up to speed and explain everything that is needed. We can parse all of fpm reliably, but there are a few issues regarding comments (we can parse most of them, but in some cases we still skip them).

Jan 07 '23 03:01 certik