Add formatting check action and apply an initial format #984

t-bltg · 2022-07-05T13:39:16Z

This PR is a proposition in order to add consistency across the source and test files (after #983 (comment)).

The tests are hard to read because the @testset blocks are not indented:

HDF5.jl/test/plain.jl

Line 438 in ccdb3e9

@testset "h5d_fill" begin

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 :

HDF5.jl/src/datasets.jl

Line 320 in ccdb3e9

    
           function create_external_dataset(parent::Union{File,Group}, name::AbstractString, filepath::AbstractString, t, sz::Dims, offset::Integer=0)

, and

HDF5.jl/src/datasets.jl

Line 332 in ccdb3e9

    
           read_dataset(dset::Dataset, memtype::Datatype, buf, xfer::DatasetTransferProperties=dset.xfer) =

making this hard to follow (inconsistent).

The declarations like:

HDF5.jl/src/groups.jl

Line 115 in ccdb3e9

    
           move_link(src::Union{File,Group}, src_name::AbstractString, dest::Union{File,Group}, dest_name::AbstractString=src_name, lapl::LinkAccessProperties = LinkAccessProperties(), lcpl::LinkCreateProperties = LinkCreateProperties()) =

are hard to read and it would be IMO better if they could be split into multiple lines such as

HDF5.jl/src/readwrite.jl

Line 247 in ccdb3e9

Base.similar(

(basically break on a specific margin / line width).

The indentation is inconsistent e.g.

HDF5.jl/src/api/helpers.jl

Line 299 in d6ecf42

return _has_h5p_set_file_locking

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. #983 (comment) or #917 (comment)).

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 · 2022-07-05T23:08:41Z

Available style codifications:

@musm seems to have style thoughts in this comment #917 (comment). Perhaps he would like to codify them via JuliaFormatter.

mkitti · 2022-07-05T23:13:39Z

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 · 2022-07-05T23:43:30Z

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

Absolutely, this can wait until the queue is low.

mkitti · 2022-07-19T16:40:45Z

I resolved the merge conflict.

t-bltg · 2022-07-19T16:56:23Z

Rebased since the format tests were failing.

t-bltg · 2022-08-03T09:57:08Z

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

musm · 2022-08-03T15:59:18Z

src/attributes.jl

    return Attribute(attrid, file(parent))
 end

 # generic method
 write_attribute(attr::Attribute, memtype::Datatype, x) = API.h5a_write(attr, memtype, x)
 # specific methods
 function write_attribute(attr::Attribute, memtype::Datatype, x::AbstractArray)
-    length(x) == length(attr) || throw(ArgumentError("Invalid length: $(length(x)) != $(length(attr)), for attribute \"$(name(attr))\""))
+    length(x) == length(attr) || throw(


This shouldn't be a one liner

You mean we should manually convert to plain if ... end, right ?

Let's do this after this pull request

mkitti · 2022-08-03T18:54:55Z

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 · 2022-08-03T20:26:01Z

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.

.github/workflows/Format-check.yml

musm · 2022-08-05T20:37:45Z

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

margin = 120

musm · 2022-08-05T20:39:31Z

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 · 2022-08-05T21:49:51Z

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 · 2022-08-05T21:52:24Z

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 · 2022-08-05T21:52:56Z

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.

mkitti

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

gen/gen_wrappers.jl

src/datasets.jl

src/dataspaces.jl

t-bltg · 2022-08-05T21:55:07Z

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

Agreed, back to blue.

t-bltg · 2022-08-05T22:09:00Z

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

Closer...

src/file.jl

test/readremote.jl

mkitti · 2022-08-05T22:06:12Z

src/attributes.jl

    return Attribute(attrid, file(parent))
 end

 # generic method
 write_attribute(attr::Attribute, memtype::Datatype, x) = API.h5a_write(attr, memtype, x)
 # specific methods
 function write_attribute(attr::Attribute, memtype::Datatype, x::AbstractArray)
-    length(x) == length(attr) || throw(ArgumentError("Invalid length: $(length(x)) != $(length(attr)), for attribute \"$(name(attr))\""))
+    length(x) == length(attr) || throw(


Let's do this after this pull request

src/file.jl

mkitti · 2022-08-05T22:12:52Z

src/properties.jl

+    if name === :local_heap_size_hint
+        API.h5p_get_local_heap_size_hint(p)
+    elseif name === :track_order
+        get_track_order(p)
+    else
+        class_getproperty(superclass(GroupCreateProperties), p, name)


I'm confused why it modified this and not some of the others below.

src/properties.jl

src/typeconversions.jl

mkitti · 2022-08-05T22:25:55Z

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 · 2022-08-05T22:53:36Z

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 · 2022-08-05T22:56:48Z

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 · 2022-08-05T23:01:24Z

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

mkitti

Approved pending CI

mkitti · 2022-08-05T23:46:25Z

I am planning to rebase and merge this.

musm · 2022-08-06T01:13:26Z

I'd recommend squash + merge next time

t-bltg · 2022-08-06T06:19:06Z

Thanks for the help and valuable comments towards better code quality 💯

mkitti · 2022-08-07T23:56:14Z

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 · 2022-08-08T00:40:22Z

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

t-bltg force-pushed the fmt branch from a0291ca to ec47d94 Compare July 5, 2022 13:39

t-bltg force-pushed the fmt branch from ec47d94 to 228e355 Compare July 6, 2022 11:07

mkitti mentioned this pull request Jul 19, 2022

Summer maintainership? #991

Closed

t-bltg force-pushed the fmt branch from 228e355 to 38f1e1d Compare July 19, 2022 08:39

t-bltg force-pushed the fmt branch from 0910ef3 to 355ea7f Compare July 19, 2022 16:55

t-bltg force-pushed the fmt branch from 355ea7f to 73eceb4 Compare July 22, 2022 09:41

t-bltg force-pushed the fmt branch 4 times, most recently from e64a8be to d30d342 Compare August 3, 2022 09:36

musm reviewed Aug 3, 2022

View reviewed changes

t-bltg force-pushed the fmt branch 2 times, most recently from db4ce0f to f21e94a Compare August 3, 2022 18:14

t-bltg force-pushed the fmt branch 2 times, most recently from 2a1ce34 to 5194686 Compare August 3, 2022 20:22

t-bltg force-pushed the fmt branch from 5194686 to 4796d05 Compare August 5, 2022 12:12

mkitti reviewed Aug 5, 2022

View reviewed changes

.github/workflows/Format-check.yml Outdated Show resolved Hide resolved

t-bltg force-pushed the fmt branch 2 times, most recently from b1d4830 to d0ccd75 Compare August 5, 2022 20:17

mkitti mentioned this pull request Aug 5, 2022

Preparations for JuliaFormatter styling, except api code gen #1001

Closed

t-bltg force-pushed the fmt branch 2 times, most recently from e24d969 to df52615 Compare August 5, 2022 21:41

add formatter action

aa80f54

turn off formatting for auto-generated files

bbd7fa8

mkitti reviewed Aug 5, 2022

View reviewed changes

gen/gen_wrappers.jl Outdated Show resolved Hide resolved

src/datasets.jl Outdated Show resolved Hide resolved

src/datasets.jl Outdated Show resolved Hide resolved

src/datasets.jl Outdated Show resolved Hide resolved

src/dataspaces.jl Outdated Show resolved Hide resolved

run initial format(["src", "test", "deps", "filters", "gen"]) x2

da7eaef

t-bltg force-pushed the fmt branch from df52615 to da7eaef Compare August 5, 2022 21:54

musm approved these changes Aug 5, 2022

View reviewed changes

mkitti reviewed Aug 5, 2022

View reviewed changes

Leave some sections up to manual formatting for now

0cdf4a6

mkitti approved these changes Aug 5, 2022

View reviewed changes

mkitti added 2 commits August 5, 2022 19:26

Fix some dos endline markers

84164f4

Remove excess endline from src/api/functions.jl

b8e6f58

mkitti merged commit 259763f into JuliaIO:master Aug 5, 2022

t-bltg deleted the fmt branch August 6, 2022 06:18

t-bltg mentioned this pull request Aug 6, 2022

cleanup test files #1003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add formatting check action and apply an initial format #984

Add formatting check action and apply an initial format #984

t-bltg commented Jul 5, 2022 •

edited

Loading

mkitti commented Jul 5, 2022

mkitti commented Jul 5, 2022

t-bltg commented Jul 5, 2022

mkitti commented Jul 19, 2022

t-bltg commented Jul 19, 2022 •

edited

Loading

t-bltg commented Aug 3, 2022

musm Aug 3, 2022

t-bltg Aug 3, 2022

musm Aug 5, 2022

mkitti Aug 5, 2022

mkitti commented Aug 3, 2022 •

edited

Loading

t-bltg commented Aug 3, 2022 •

edited

Loading

musm commented Aug 5, 2022

musm commented Aug 5, 2022 •

edited

Loading

t-bltg commented Aug 5, 2022

musm commented Aug 5, 2022

musm commented Aug 5, 2022

mkitti left a comment

t-bltg commented Aug 5, 2022 •

edited

Loading

t-bltg commented Aug 5, 2022 •

edited

Loading

mkitti left a comment

mkitti Aug 5, 2022

mkitti Aug 5, 2022

mkitti commented Aug 5, 2022

mkitti commented Aug 5, 2022

musm commented Aug 5, 2022

mkitti commented Aug 5, 2022

mkitti left a comment

mkitti commented Aug 5, 2022

musm commented Aug 6, 2022

t-bltg commented Aug 6, 2022

mkitti commented Aug 7, 2022

mkitti commented Aug 8, 2022

Add formatting check action and apply an initial format #984

Add formatting check action and apply an initial format #984

Conversation

t-bltg commented Jul 5, 2022 • edited Loading

mkitti commented Jul 5, 2022

mkitti commented Jul 5, 2022

t-bltg commented Jul 5, 2022

mkitti commented Jul 19, 2022

t-bltg commented Jul 19, 2022 • edited Loading

t-bltg commented Aug 3, 2022

musm Aug 3, 2022

Choose a reason for hiding this comment

t-bltg Aug 3, 2022

Choose a reason for hiding this comment

musm Aug 5, 2022

Choose a reason for hiding this comment

mkitti Aug 5, 2022

Choose a reason for hiding this comment

mkitti commented Aug 3, 2022 • edited Loading

t-bltg commented Aug 3, 2022 • edited Loading

musm commented Aug 5, 2022

musm commented Aug 5, 2022 • edited Loading

t-bltg commented Aug 5, 2022

musm commented Aug 5, 2022

musm commented Aug 5, 2022

mkitti left a comment

Choose a reason for hiding this comment

t-bltg commented Aug 5, 2022 • edited Loading

t-bltg commented Aug 5, 2022 • edited Loading

mkitti left a comment

Choose a reason for hiding this comment

mkitti Aug 5, 2022

Choose a reason for hiding this comment

mkitti Aug 5, 2022

Choose a reason for hiding this comment

mkitti commented Aug 5, 2022

mkitti commented Aug 5, 2022

musm commented Aug 5, 2022

mkitti commented Aug 5, 2022

mkitti left a comment

Choose a reason for hiding this comment

mkitti commented Aug 5, 2022

musm commented Aug 6, 2022

t-bltg commented Aug 6, 2022

mkitti commented Aug 7, 2022

mkitti commented Aug 8, 2022

t-bltg commented Jul 5, 2022 •

edited

Loading

t-bltg commented Jul 19, 2022 •

edited

Loading

mkitti commented Aug 3, 2022 •

edited

Loading

t-bltg commented Aug 3, 2022 •

edited

Loading

musm commented Aug 5, 2022 •

edited

Loading

t-bltg commented Aug 5, 2022 •

edited

Loading

t-bltg commented Aug 5, 2022 •

edited

Loading