Add formatting check action and apply an initial format

[t-bltg]

This PR is a proposition in order to add consistency across the source and test files (after https://github.com/JuliaIO/HDF5.jl/pull/983#issuecomment-1175011022).

The tests are hard to read because the @testset blocks are not indented: https://github.com/JuliaIO/HDF5.jl/blob/ccdb3e90a5910bee954d066de7b3f775f9067bc3/test/plain.jl#L438 requiring (spurious) manual addition of comment such as end # testset "Test h5d_fill.

The source files mixes both function ... end and one liner function declarations like : https://github.com/JuliaIO/HDF5.jl/blob/ccdb3e90a5910bee954d066de7b3f775f9067bc3/src/datasets.jl#L320 , and https://github.com/JuliaIO/HDF5.jl/blob/ccdb3e90a5910bee954d066de7b3f775f9067bc3/src/datasets.jl#L332 making this hard to follow (inconsistent).

The declarations like: https://github.com/JuliaIO/HDF5.jl/blob/ccdb3e90a5910bee954d066de7b3f775f9067bc3/src/groups.jl#L115 are hard to read and it would be IMO better if they could be split into multiple lines such as https://github.com/JuliaIO/HDF5.jl/blob/ccdb3e90a5910bee954d066de7b3f775f9067bc3/src/readwrite.jl#L247 (basically break on a specific margin / line width).

The indentation is inconsistent e.g. https://github.com/JuliaIO/HDF5.jl/blob/d6ecf42d035784f78616a24b03cda7c037eda7c9/src/api/helpers.jl#L299 which uses 3 spaces instead of 4 (https://docs.julialang.org/en/v1/manual/style-guide/).

I'll stop there with the examples, my point being that I don't care which style convention is used (this is very opinionated), as long as it is consistent across all source files.

Currently in this repo, no style guidelines are provided for developers, inducing wasted time on style discussions (e.g. https://github.com/JuliaIO/HDF5.jl/pull/983#issuecomment-1175011022 or https://github.com/JuliaIO/HDF5.jl/pull/917#discussion_r830190912).

If needed, formatting auto-generated files can be omitted by adding #! format: off to a file header (e.g. gen/gen_wrappers.jl -> src/api/functions.jl) ?

I propose that we only apply formatting to src and test directories (for the end users), but this could be extended to other directories like gen (to be discussed ?).

All the current inconsistencies can be handled in an automated manner using a formatting check action (auto format PRs should be avoided, because they pollute the git history: we should instead use (blocking or non blocking) checks for PRs, see the added .github/workflows/Format-check.yml action).

• [ ] discuss which stylistic conventions are to be used (I'd vote for BlueStyle + tweaks in .JuliaFormatter).

[mkitti]

Available style codifications:

• JuliaFormatter.BlueStyle: https://github.com/invenia/BlueStyle#method-definitions
• YASGuide: https://github.com/jrevels/YASGuide
• SciML style: https://github.com/SciML/SciMLStyle

@musm seems to have style thoughts in this comment https://github.com/JuliaIO/HDF5.jl/pull/917#discussion_r830190912. Perhaps he would like to codify them via JuliaFormatter.

[mkitti]

To merge this, I would like to try to clear the pull request queue as much as possible. Merging this will require significant work to make those branches work again.

[t-bltg]

Merging this will require significant work to make those branches work again.

Absolutely, this can wait until the queue is low.

[mkitti]

I resolved the merge conflict.

[t-bltg]

Rebased since the format tests were failing.

[t-bltg]

I think it's time to go forward on this one (recent PR queue is low). @musm, what do you think ?

[mkitti]

For this pull request, we should primarily focus on getting the settings to .JuliaFormatter.toml correct. I should be able to pull master, check out .JuliaFormatter.toml, apply the formatting, and see almost no diff to this branch. Other stylistic changes should be reserved for other pull requests.

[t-bltg]

I should be able to pull master, check out .JuliaFormatter.toml, apply the formatting, and see almost diff to this branch.

This should be the case (I've renamed the 2nd commit to show that I only ran format(["src", "test"]) twice after creating the .JuliaFormatter configuration file (1st commit in this PR)).

Why twice ? To check that formatting is idempotent, so that we don't run into formatting subtleties.

[musm]

can we increase the default line length to 120 or 140? I think the following addition is what's needed.

margin = 120

[musm]

My personal preference is for YAS instead of the blue style wrt to the alignment of functions and nesting: https://domluna.github.io/JuliaFormatter.jl/stable/yas_style/#Differences-from-DefaultStyle

[t-bltg]

can we increase the default line length to 120 or 140?

Changed to 120.

My personal preference is for YAS

Style set to yas.

[mkitti]

We need to turn off formatting for gen/apidefs.jl and src/api/functions.jl. I've done this in #1001 .

[t-bltg]

The margin=120 looks odd. I'd stick to the recommended 92 chars margin.

[musm]

I think you might be right on it being too long.

[t-bltg]

I think you might be right on it being too long.

Back to 92.

[musm]

Anyone else want to chime in on their preference, as mentioned I think I have a slight preference to the YAS style.

In general, I'm not crazy about auto-formatters I think sometimes it makes code less clear than originally written, but I appreciate the consistency.

[t-bltg]

In general, I'm not crazy about auto-formatters I think sometimes it makes code less clear than originally written, but I appreciate the consistency.

They are not perfect, but imo the worst you can do is being inconsistent across the repo.

[musm]

I'm not against this change, but I disagree in that I don't think inconsistency is the worst you can do. Base Julia is inconsistent across the whole repo and the style does vary depending on who wrote what.

[musm]

In general, everything looks fine, I'm just debating if we should switch back to blue, some of the changes with YAS are indeed annoying.

[t-bltg]

Some of the indentation looks excessive. I think this is due to YAS.

Agreed, back to blue.

[t-bltg]

Base Julia is inconsistent across the whole repo and the style does vary depending on who wrote what.

Exactly, JuliaLang/julia should have a format auto-check for PRs, and stick to an arbitrary style. Those are good coding practices everyone should adopt. Unfortunately, a PR to julia would be pointless (0% chance of being merged) given the flame wars it would trigger.

[mkitti]

Maybe we should just merge and revert some of this manually. I think we should also add a preformatting tag unless we want to release before merging this.

[mkitti]

Ok, I think we're close enough. I'm going to make a few manual changes to turn off formatting in some sections, and then merge.

[musm]

Ok, I think we're close enough. I'm going to make a few manual changes to turn off formatting in some sections, and then merge.

SGTM

Do we need a pre-tag, maybe just the commit is good enough.

[mkitti]

We just need a easy to remember reference for "before_formatting" in case we find other situations where the formatting got worse.

[mkitti]

I am planning to rebase and merge this.

[musm]

I'd recommend squash + merge next time

[t-bltg]

Thanks for the help and valuable comments towards better code quality :100:

[mkitti]

I'd recommend squash + merge next time

I intentionally chose not to squash here since t-bltg explicitly tried to divide up his work into three distinct, atomic commits. Perhaps I should have squashed my commits together.

[mkitti]

By the way, it seems that HDF5 upstream also recently started doing automatic formatting using clang-format: https://github.com/HDFGroup/hdf5/blob/develop/.clang-format