apply JuliaFormatter.jl #536

simonbyrne · 2019-11-14T00:54:55Z

This applies JuliaFormatter.jl to the whole repository.

Any thoughts/comments?

Testing?

jkozdon · 2019-11-14T16:23:27Z

I like this idea a lot.

Suggestions:

Can you post instructions on how to set up githooks to check this?
Can we set up the CI to reject non-formatted code? (There is some github actions for this, but I don't know much about these)
Add something to README on local usage?
Is there some style file? I didn't see anything in the commit to tell me how to do this on my commits or what options to set.

vchuravy · 2019-11-14T17:19:27Z

I am a huge fan of this, but I would prefer an incremental approach where every new/change code has to go through the formateé. Otherwise we have a hard break in the got history.

On the other hand just doing it might be a lot less work.

simonbyrne · 2019-11-14T17:27:24Z

I posted this commit just to see if people liked the output. If we're happy with how it looks, I'll add instructions, CI, docs, etc.

* Is there some style file? I didn't see anything in the commit to tell me how to do this on my commits or what options to set.

This was just run using the default settings (indent=4, margin=92). Unfortunately it doesn't appear to have a whole lot of other options.

I am a huge fan of this, but I would prefer an incremental approach where every new/change code has to go through the formateé. Otherwise we have a hard break in the got history.

On the other hand just doing it might be a lot less work.

Yeah, not sure there is an obvious way to make it incremental. Also, since it changes the indentation from 2 to 4, it would be pretty hard to do incrementally.

blallen · 2019-11-14T17:35:41Z

Do we need to switch the indentation from 2 to 4?

blallen · 2019-11-14T17:37:53Z

src/DGmethods/DGmodel.jl

+        @launch(
+            device,
+            threads = Nfp,
+            blocks = nrealelem,
+            faceviscterms!(
+                bl,
+                Val(dim),
+                Val(polyorder),
+                dg.direction,
+                dg.gradnumflux,
+                Q.data,
+                Qvisc.data,
+                auxstate.data,
+                vgeo,
+                sgeo,
+                t,
+                vmapM,
+                vmapP,
+                elemtobndy,
+                topology.realelems,
+            )
+        )


My main reservations about the formatter is that it makes some of the function calls that used to fit on 2-3 lines take 20 lines. While this is cleaner, it also means you see less of the algorithm on your screen at once.

Is there a way to make it only linebreak after the last comma before hitting 92 characters on a line instead of after every comma?

No, unfortunately. Personally I feel this is the main downside as well.

For particularly bad places there is always skip formatting

simonbyrne · 2019-11-14T17:47:25Z

Do we need to switch the indentation from 2 to 4?

No this is one of the few options the formatter provides, but it had been suggested by several people as a way to make the nested function calls more obvious. The cases where we have large numbers of nested loops such as
https://github.com/climate-machine/CLIMA/blob/4e72b21ddcc200c998dba964b9af6354ffeaaddc/src/DGmethods/DGmodel_kernels.jl#L99-L116
might be better handled with a macro similar to Oceaninigans.jl:
https://github.com/climate-machine/Oceananigans.jl/blob/866696432a5214ec5f927bf21835f072a8691c78/src/utils.jl#L13-L24

But again, feedback appreciated.

jkozdon · 2019-11-14T17:58:53Z

This was just run using the default settings (indent=4, margin=92).

Personal I like 2 spaces and 80 character limit, but much prefer uniformity than these little things.

simonbyrne · 2020-02-25T17:25:24Z

So the general consensus is that we should enable this. The main concerns:

This will break all incoming PRs, which will require a huge amount of work to get back into shape

@vchuravy will look to see if we can do this incrementally?

How should it be implemented:

There is a github action which applies it as a check: https://github.com/julia-actions/julia-format, which would require users to manually use the formatter.
Ideally it would be applied to PRs directly, but there doesn't seem to be a way to do this at the moment.

Plan:

We will enable check for the time being, but don't enforce that it passes
If PRs make significant changes to a file, then they should apply the formatter to that file.

The remaining question is what formatting options should be applied? If you have opinions chime in below. The options are here: https://github.com/domluna/JuliaFormatter.jl

lcw · 2020-02-25T18:01:46Z

It would also be nice to have a git hook that runs the formatter. Below is what we used in a previous project as a pre-commit hook.

#!/bin/bash
#
# Called by git-commit with no arguments.  This checks to make
# sure that all .c and .h files are indented correctly before
# a commit is made.
#
# To enable this hook, make this file executable and place it
# in $GIT_DIR/hooks.

. git-sh-setup

CHFILES=$(git-diff-index -M --name-only --cached HEAD | \
          grep -E '.*\.(c|cpp|h)$' | grep -v 'node_data\.h')

for CHFILE in $CHFILES;
do
  MKTEMPLATE=`basename $CHFILE`.XXXXXXXX
  TEMPFILE=`mktemp -t "$MKTEMPLATE"` || exit 1
  clang-format $GIT_DIR/../$CHFILE > $TEMPFILE
  if diff $GIT_DIR/../$CHFILE $TEMPFILE
  then
    rm -f $TEMPFILE
  else
    rm -f $TEMPFILE
    NEEDS_FORMAT=1
    echo >&2 "$CHFILE needs to be indented with:"
    echo >&2 "   clang-format -i $GIT_DIR/../$CHFILE"
  fi
done
if [ -z "$NEEDS_FORMAT" ]
then
  exit 0
else
  exit 1
fi

lcw · 2020-02-25T18:03:25Z

I also like having a formal style guide, such as https://github.com/jrevels/YASGuide, which JuliaFormatter.jl almost fully supports.

blallen · 2020-02-26T01:02:03Z

The remaining question is what formatting options should be applied? If you have opinions chime in below. The options are here: https://github.com/domluna/JuliaFormatter.jl

my opinions:

indent = 2
margin = 80 (I like to split my screen into four vertical slices)
always_for_in = true
whitespace_typedefs = true
whitespace_ops_in_indices = true
remove_extra_newlines = false

simonbyrne · 2020-02-26T01:16:53Z

I'm not too fussy: I generally use 4 space indent elsewhere, but our kernels have a lot of indentation which makes it cumbersome.

leios · 2020-02-26T18:53:03Z

I agree with almost all the points set in the Coding Conventions section of the documentation. Switching to 80 columns (instead of 79) is a minor change. Outside of that, it looks like we can follow the YASguide with small exceptions for allowed unicode characters and using 2 spaces instead of 4.

The formatter will fix all the spacing, but we need to make sure we are consistent with function names and everything else as well, and that might need to be left to the reviewer(s).

I think I agree with @blallen's list, except that I would set remove_extra_newlines = true. I think this option only removes newlines when there are 2 or more consecutive newines.

Once we have this decided, I will update the Coding Conventions document to reflect the changes here.

kpamnany · 2020-03-02T16:44:10Z

I vote for 4 spaces. The point of indentation is for a clear visual demarcation of blocks, which also means variable scope in many cases. Less than 4 spaces is not clear, IMO.

Apart from that, I'm good with everything else.

simonbyrne · 2020-03-12T05:41:18Z

I've updated this PR to the current changes.

Once this is merged, the process to update an existing PR will look something like:

git checkout master .dev/format.jl # to get the most recent formatter script
.dev/format.jl
.dev/format.jl # need to apply twice
git add -u
git commit -m "apply formatter"
git merge master # this should be a relatively clean merge

To squash this all into a single commit, you should then be able to

git reset --soft master
git commit -m "my new commit message"

simonbyrne · 2020-03-12T05:42:47Z

Please chime in if you have any objections!

simonbyrne · 2020-03-12T20:23:43Z

I might try to do this over the weekend.

leios

I was scrolling through most of the files and it looks fine for the most part. Some of the manual alignment (for example when aligning = signs) is now gone and comments are funky. JuliaFormatter will not allow us to do manual alignment anymore, but maybe it's worth fixing some of the comments while we are at it?

leios · 2020-03-12T20:49:29Z

src/Atmos/Parameterizations/SurfaceFluxes/SurfaceFluxes.jl

+    s_b = Ri / Pr_0
+    if Ri > 0
+        temp = ((1 - 2 * β_h * Ri) - sqrt(1 + 4 * (β_h - β_m) * s_b))
+        ζ = zfactor / (2 * β_h * (β_m * Ri - 1)) * temp                    # Eq. 19 in Ref. Byun1990


The comments are super weird in this area (and a few other areas) due to JuliaFormatter not dealing with comments.

This stuff will be gone when the new surface fluxes is merged (#556)

jkozdon · 2020-03-19T15:05:59Z

Some of the manual alignment (for example when aligning = signs) is now gone and comments are funky. JuliaFormatter will not allow us to do manual alignment anymore, but maybe it's worth fixing some of the comments while we are at it?

You can put stuff in blocks to turn off the formatter:

#! format: off

Stuff in here the formatter skips

#! format: on

leios · 2020-03-19T15:29:33Z

Should we go through and figure out where all the manual formatting is preferred, set syntax off and on before merging? I guess we should at least rebase and apply the formatter again before merging this.

simonbyrne · 2020-03-19T15:54:19Z

If someone wants to figure out how JuliaFormatter works, we could modify it to suit our own needs.

simonbyrne · 2020-03-23T16:32:39Z

bors r+

536: apply JuliaFormatter.jl r=simonbyrne a=simonbyrne This applies [JuliaFormatter.jl](https://github.com/domluna/JuliaFormatter.jl) to the whole repository. Any thoughts/comments? - [ ] Testing? Co-authored-by: Simon Byrne <simonbyrne@gmail.com>

bors · 2020-03-23T18:14:31Z

Build failed

ci/slurmci

simonbyrne · 2020-03-24T02:52:26Z

bors r+

536: apply JuliaFormatter.jl r=simonbyrne a=simonbyrne This applies [JuliaFormatter.jl](https://github.com/domluna/JuliaFormatter.jl) to the whole repository. Any thoughts/comments? - [ ] Testing? Co-authored-by: Simon Byrne <simonbyrne@gmail.com>

bors · 2020-03-24T05:00:57Z

Build failed

ci/slurmci

codecov · 2020-03-25T04:08:27Z

Codecov Report

Merging #536 into master will decrease coverage by 0.03%.
The diff coverage is 69.33%.

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
- Coverage   70.55%   70.51%   -0.04%     
==========================================
  Files         108      108              
  Lines        6717     6736      +19     
==========================================
+ Hits         4739     4750      +11     
- Misses       1978     1986       +8

Impacted Files	Coverage Δ
...mos/Parameterizations/CloudPhysics/Microphysics.jl	`95.45% <ø> (ø)`	⬆️
.../Common/MoistThermodynamics/MoistThermodynamics.jl	`79.71% <ø> (ø)`	⬆️
src/Common/PlanetParameters/PlanetaryConstants.jl	`100% <ø> (ø)`	⬆️
src/Driver/Driver.jl	`81.72% <ø> (ø)`	⬆️
src/DGmethods_old/DGBalanceLawDiscretizations.jl	`0.59% <ø> (ø)`	⬆️
src/Mesh/Interpolation.jl	`76.28% <ø> (ø)`	⬆️
src/Common/ConfigTypes/ConfigTypes.jl	`100% <ø> (ø)`	⬆️
src/Arrays/CMBuffers.jl	`14.81% <0%> (ø)`	⬆️
...methods_old/DGBalanceLawDiscretizations_kernels.jl	`0% <0%> (ø)`	⬆️
src/Common/PlanetParameters/EarthConstants.jl	`0% <0%> (ø)`	⬆️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f978f6f...b8cee51. Read the comment docs.

simonbyrne · 2020-03-25T04:39:49Z

bors r+

536: apply JuliaFormatter.jl r=simonbyrne a=simonbyrne This applies [JuliaFormatter.jl](https://github.com/domluna/JuliaFormatter.jl) to the whole repository. Any thoughts/comments? - [ ] Testing? Co-authored-by: Simon Byrne <simonbyrne@gmail.com>

bors · 2020-03-25T06:20:41Z

Build failed

ci/slurmci

simonbyrne · 2020-05-11T16:17:58Z

bors r+

bors · 2020-05-11T22:01:11Z

Build succeeded:

blallen reviewed Nov 14, 2019

View reviewed changes

simonbyrne force-pushed the sb/formatter branch from 4e72b21 to f8ea8ca Compare November 17, 2019 03:47

simonbyrne force-pushed the master branch from 61364b7 to 5f2f29e Compare November 17, 2019 03:51

blallen added the Julia label Jan 7, 2020

simonbyrne force-pushed the sb/formatter branch from f8ea8ca to 85a86d0 Compare March 12, 2020 05:28

simonbyrne marked this pull request as ready for review March 12, 2020 05:28

simonbyrne force-pushed the sb/formatter branch from 85a86d0 to cab981c Compare March 12, 2020 05:30

simonbyrne mentioned this pull request Mar 12, 2020

Formatting as separate commit #830

Closed

3 tasks

kpamnany mentioned this pull request Mar 12, 2020

Fix RemainderModel boundary_state! #838

Closed

4 tasks

leios reviewed Mar 12, 2020

View reviewed changes

simonbyrne force-pushed the sb/formatter branch from cab981c to ba69319 Compare March 23, 2020 16:30

simonbyrne force-pushed the sb/formatter branch from ba69319 to a2752f3 Compare March 24, 2020 02:52

simonbyrne force-pushed the sb/formatter branch from a2752f3 to b8cee51 Compare March 25, 2020 04:08

simonbyrne added this to the v 0.1.0 release milestone Apr 9, 2020

kpamnany linked an issue Apr 10, 2020 that may be closed by this pull request

Formatting as separate commit #830

Closed

3 tasks

apply formatting to remaining files

a1adb06

simonbyrne force-pushed the sb/formatter branch from b8cee51 to a1adb06 Compare May 11, 2020 04:44

simonbyrne added 2 commits May 11, 2020 08:53

test error type instead of exact message

75c5b08

remove .dev/format.jl

c7e60a9

bors bot merged commit 3e9471a into master May 11, 2020

bors bot deleted the sb/formatter branch May 11, 2020 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply JuliaFormatter.jl #536

apply JuliaFormatter.jl #536

simonbyrne commented Nov 14, 2019 •

edited by leios

Loading

jkozdon commented Nov 14, 2019

vchuravy commented Nov 14, 2019

simonbyrne commented Nov 14, 2019

blallen commented Nov 14, 2019

blallen Nov 14, 2019 •

edited

Loading

simonbyrne Nov 14, 2019

jkozdon Nov 14, 2019

simonbyrne commented Nov 14, 2019

jkozdon commented Nov 14, 2019

simonbyrne commented Feb 25, 2020

lcw commented Feb 25, 2020

lcw commented Feb 25, 2020

blallen commented Feb 26, 2020

simonbyrne commented Feb 26, 2020

leios commented Feb 26, 2020

kpamnany commented Mar 2, 2020

simonbyrne commented Mar 12, 2020

simonbyrne commented Mar 12, 2020

simonbyrne commented Mar 12, 2020

leios left a comment

leios Mar 12, 2020

charleskawczynski Mar 19, 2020

jkozdon commented Mar 19, 2020

leios commented Mar 19, 2020

simonbyrne commented Mar 19, 2020

simonbyrne commented Mar 23, 2020

bors bot commented Mar 23, 2020

simonbyrne commented Mar 24, 2020

bors bot commented Mar 24, 2020

codecov bot commented Mar 25, 2020 •

edited

Loading

simonbyrne commented Mar 25, 2020

bors bot commented Mar 25, 2020

simonbyrne commented May 11, 2020

bors bot commented May 11, 2020

apply JuliaFormatter.jl #536

apply JuliaFormatter.jl #536

Conversation

simonbyrne commented Nov 14, 2019 • edited by leios Loading

jkozdon commented Nov 14, 2019

vchuravy commented Nov 14, 2019

simonbyrne commented Nov 14, 2019

blallen commented Nov 14, 2019

blallen Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

simonbyrne Nov 14, 2019

Choose a reason for hiding this comment

jkozdon Nov 14, 2019

Choose a reason for hiding this comment

simonbyrne commented Nov 14, 2019

jkozdon commented Nov 14, 2019

simonbyrne commented Feb 25, 2020

lcw commented Feb 25, 2020

lcw commented Feb 25, 2020

blallen commented Feb 26, 2020

simonbyrne commented Feb 26, 2020

leios commented Feb 26, 2020

kpamnany commented Mar 2, 2020

simonbyrne commented Mar 12, 2020

simonbyrne commented Mar 12, 2020

simonbyrne commented Mar 12, 2020

leios left a comment

Choose a reason for hiding this comment

leios Mar 12, 2020

Choose a reason for hiding this comment

charleskawczynski Mar 19, 2020

Choose a reason for hiding this comment

jkozdon commented Mar 19, 2020

leios commented Mar 19, 2020

simonbyrne commented Mar 19, 2020

simonbyrne commented Mar 23, 2020

bors bot commented Mar 23, 2020

Build failed

simonbyrne commented Mar 24, 2020

bors bot commented Mar 24, 2020

Build failed

codecov bot commented Mar 25, 2020 • edited Loading

Codecov Report

simonbyrne commented Mar 25, 2020

bors bot commented Mar 25, 2020

Build failed

simonbyrne commented May 11, 2020

bors bot commented May 11, 2020

simonbyrne commented Nov 14, 2019 •

edited by leios

Loading

blallen Nov 14, 2019 •

edited

Loading

codecov bot commented Mar 25, 2020 •

edited

Loading