Formatting

[baumgold]

I used the default JuliaFormatter options, but we can customize them if we want to with a .JuliaFormatter.toml file. Details available here:

https://domluna.github.io/JuliaFormatter.jl

Fixes #398. CC: @svilupp

[codecov-commenter]

Codecov Report

Merging #464 (420df67) into main (5532270) will decrease coverage by 82.60%. The diff coverage is 8.21%.

@@            Coverage Diff             @@
##             main    #464       +/-   ##
==========================================
- Coverage   87.53%   4.93%   -82.60%     
==========================================
  Files          26      25        -1     
  Lines        3272    3199       -73     
==========================================
- Hits         2864     158     -2706     
- Misses        408    3041     +2633     

Impacted Files	Coverage Δ
src/Arrow.jl	0.00% <0.00%> (-97.44%)	:arrow_down:
src/FlatBuffers/FlatBuffers.jl	0.00% <0.00%> (-93.34%)	:arrow_down:
src/FlatBuffers/builder.jl	0.00% <0.00%> (-80.12%)	:arrow_down:
src/FlatBuffers/table.jl	0.00% <0.00%> (-60.00%)	:arrow_down:
src/append.jl	0.00% <0.00%> (-92.05%)	:arrow_down:
src/arraytypes/arraytypes.jl	0.00% <0.00%> (-90.00%)	:arrow_down:
src/arraytypes/bool.jl	0.00% <0.00%> (-88.71%)	:arrow_down:
src/arraytypes/compressed.jl	0.00% <0.00%> (-100.00%)	:arrow_down:
src/arraytypes/dictencoding.jl	0.00% <0.00%> (-75.59%)	:arrow_down:
src/arraytypes/fixedsizelist.jl	0.00% <0.00%> (-84.54%)	:arrow_down:
... and 15 more

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[quinnj]

I think this is mostly great and I agree we should do it. I think the only change I'm really against is removing spaces around operators, regardless of where or what kind of clause they occur in. I also personally prefer keeping spaces around the <: operator in type parameter clauses, but I don't feel as strongly about that if people prefer otherwise.

[quinnj]

@baumgold, can you clarify the CI change here? Does it automatically add a commit w/ formatting changes to your PR? Or post suggestions that can be committed?

[baumgold]

I think this is mostly great and I agree we should do it. I think the only change I'm really against is removing spaces around operators, regardless of where or what kind of clause they occur in. I also personally prefer keeping spaces around the <: operator in type parameter clauses, but I don't feel as strongly about that if people prefer otherwise.

I believe the only control for this that we are given in JuliaFormatter is the following:

https://github.com/domluna/JuliaFormatter.jl#whitespace_ops_in_indices

I overrode the default from false to true. That seems like it should mostly fix this issue.

[baumgold]

@baumgold, can you clarify the CI change here? Does it automatically add a commit w/ formatting changes to your PR? Or post suggestions that can be committed?

I think the CI change can be simplified using the "julia-format" GitHub action. I'll try to get this setup shortly.

https://github.com/julia-actions/julia-format

[baumgold]

I was trying to using the julia-format GitHub action but I can't seem to get it to work. Instead I took inspiration from the following example but simplified it as much as possible for use here.

https://github.com/julia-actions/julia-format/blob/master/workflows/format_check.yml

The expected behavior is that if a PR is opened with malformed code then the format build will fail indicating that the PR should not be merged until the formatting is fixed.

[ericphanson]

Something I often do at work is check in a format directory with a Project and Manifest, a small run.jl script that does the formatting and a readme to explain how. Then the CI job can use the same project/manifest to do the check. This can allow folks to easily run the formatting locally and also ensure that JuliaFormatter updates don’t suddenly cause the CI check to fail on unrelated PRs. (Then if eg a new contributor’s PR fails the format check there is an easy remedy for them - to run the local run script in the format project).

One needs to manually PR the format manifest to update it for newer JuliaFormatter versions but imo that’s worth it being explicit (and you only need to update if there’s some specific change you want).

This may be less necessary as JuliaFormatter has gotten more stable over the years.

[baumgold]

@ericphanson - That script you speak of is embedded in the ci.yml file:

https://github.com/apache/arrow-julia/pull/464/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR165-R179

IMHO it's quite a trivial script. We can explicitly pin to a version of JuliaFormatter if we want but that adds extra maintenance overhead so would prefer not to unless there's a good reason. What do you think? Is this sufficient?

[baumgold]

@quinnj / @ericphanson - shall I merge? Any objections?

[quinnj]

Go for it