Upgrade to ocamlformat 0.26.0 #8064

Julow · 2023-06-27T16:36:32Z

The changed formatting of @@ let ... (any operator) cause huge diffs in the command descriptions. The changed indentation of if and let in parentheses also has a big impact.

The aim of this commit is to gather feedback.

Changelog can be found here: https://github.com/ocaml-ppx/ocamlformat/blob/main/CHANGES.md

emillon · 2023-06-28T07:24:23Z

Once 3.9.0 is out I'll upgrade to 0.25.0 first so we can see what's going on more precisely (I'll also enable .git-blame-ignore-revs).

rgrinberg · 2023-06-28T07:51:08Z

src/dune_rules/dune_env.ml

@@ -232,8 +233,9 @@ module Stanza = struct
    enter
      (let+ pat =
         keyword "_" >>> return Any
-         <|> let+ p = Profile.decode in
-             Profile p
+         <|>


IMO the old code looked better here.

What do you think of the similar change in cmd code, where the let-expression is much bigger and the operators are not chained ?

The old version is better there too. The additional line only pays off when the let expression has some ugly wrapping due to the lack of horizontal space. There was no such issue there.

Can ocamlformat pick the one that causes the least wrapping? (at least under the hood)

Not really, OCamlformat cannot compare two possible formatting. Only the input can be considered (though, multiple iterations on the whole code can be made).
But it can format differently whether there would be no wrapping at all and it can detect chained operators.

Such formatting could be possible:

<|> let+ p = Profile.decode in Profile p <|> ...

falling back to this as soon as it doesn't fit on the line:

<|> let+ p = Profile.decode in (* longer code *) Profile p <|> ...

I'm mentioning chained operators because to me, the cmd code looks better with less indentation.
To me, that indentation can cause ugly wrapping means that it does.

By looking at the diff again, I find that the reduced indentation do save lines by removing some wrapping. I also do not find a lot of chained operators so my previous suggestion is not valid.

Though, it should be possible have this formatting, as long as it fits on one line:

... <|> let+ p = Profile.decode in Profile p

Should I add this before the next release ?

not a huge fan of saving lines at all cost, I think that the old version was better.

I went back and found the reason for this change: ocaml-ppx/ocamlformat#2315
It's a consistency change, in the previous version, the line was broken on regular lets but not letops:

let _ = Cmd.v info @@ let+ trimmed_size = () in () let _ = () <|> let+ p = Profile.decode in Profile p let _ = Cmd.v info @@ let trimmed_size = () in () let _ = () <|> let p = Profile.decode in Profile p

The other form exists in the code: bin/describe/describe_workspace.ml line 378.

This diff can go away by keeping the line break from the source code, whether it's a let or a letop. (ocaml-ppx/ocamlformat#2396)
We try to add as few of these as possible but the regression is large enough to justify it.

I'll merge this patch and update this preview PR after the weekend.

Julow · 2023-06-28T08:15:11Z

Once 3.9.0 is out I'll upgrade to 0.25.0 first so we can see what's going on more precisely (I'll also enable .git-blame-ignore-revs).

Let me know and I'll rebase this.

bin/cache.ml

src/dune_rules/dune_project.ml

emillon · 2023-07-05T15:48:49Z

Let me know and I'll rebase this.

you can go ahead!

Julow · 2023-07-10T10:21:09Z

I rebased and re-ran the preview with the @@ let+ fix.

emillon · 2023-07-10T12:05:06Z

.git-blame-ignore-revs

@@ -1,2 +1,4 @@
 # ocamlformat 0.25.1
 3f01f6f3694e48cb1868018121f3959f8d23baca
+# Upgrade to OCamlformat 0.26.0
+8a1a919e3895787ae3781da1d6aff4cc41ce04c3


No need to do that in this PR since this will change

Right!

I'll make sure to remove that and fix the version line before I mark the PR as ready.

Though, do you think this PR could be merged unsquashed ?

for now I prefer doing this in 2 steps

.ocamlformat

bin/common.mli

emillon

In my opinion, these are all improvements. Let's wait for @rgrinberg's opinion as well.

Julow · 2023-07-18T09:47:31Z

Thanks :) Unless a problem is found, I intent to release today.

Julow · 2023-07-20T10:35:09Z

OCamlformat 0.26.0 has been released! This PR can be merged or closed if it's a bit too early to upgrade.

emillon · 2023-07-31T16:19:39Z

I rebased this; this can land once #8311 is merged.

Signed-off-by: Jules Aguillon <jules@j3s.fr> Signed-off-by: Etienne Millon <me@emillon.org>

Alizter · 2023-08-01T08:21:53Z

src/dune_rules/install_rules.ml

+                 ~compare:(fun
+                     (x : _ Install.Entry.t) (y : _ Install.Entry.t) ->
+                   Path.compare x.src y.src)


Indentation here is kind of funny. Is this expected @Julow?

It is expected: The -> was moved to the same line as the arguments to avoid the previous indentation. The arguments no longer fit on the line and have to break.

The arguments are indented more than the body for disambiguation.

Alizter

LGTM, just a non-blocking comment.

Julow marked this pull request as draft June 27, 2023 16:49

rgrinberg reviewed Jun 28, 2023

View reviewed changes

Julow commented Jul 5, 2023

View reviewed changes

bin/cache.ml Outdated Show resolved Hide resolved

Julow commented Jul 5, 2023

View reviewed changes

src/dune_rules/dune_project.ml Outdated Show resolved Hide resolved

Julow force-pushed the preview-ocamlformat-0.26.0 branch from a17d7ed to 15583f7 Compare July 10, 2023 10:13

emillon reviewed Jul 10, 2023

View reviewed changes

.ocamlformat Outdated Show resolved Hide resolved

emillon reviewed Jul 10, 2023

View reviewed changes

bin/common.mli Show resolved Hide resolved

Julow force-pushed the preview-ocamlformat-0.26.0 branch from 15583f7 to 96f2092 Compare July 18, 2023 09:17

emillon approved these changes Jul 18, 2023

View reviewed changes

Julow force-pushed the preview-ocamlformat-0.26.0 branch from 96f2092 to 272a7b1 Compare July 20, 2023 10:23

Julow changed the title ~~Preview: Upgrade to ocamlformat 0.26.0 (unreleased)~~ Upgrade to ocamlformat 0.26.0 Jul 20, 2023

Julow marked this pull request as ready for review July 20, 2023 10:35

Julow force-pushed the preview-ocamlformat-0.26.0 branch from 272a7b1 to b3a1db3 Compare July 20, 2023 10:36

rgrinberg approved these changes Jul 23, 2023

View reviewed changes

emillon force-pushed the preview-ocamlformat-0.26.0 branch from b3a1db3 to 400356c Compare July 31, 2023 16:16

Upgrade to OCamlformat 0.26.0

a18c6fc

Signed-off-by: Jules Aguillon <jules@j3s.fr> Signed-off-by: Etienne Millon <me@emillon.org>

emillon force-pushed the preview-ocamlformat-0.26.0 branch from 400356c to a18c6fc Compare July 31, 2023 16:20

Merge branch 'main' into preview-ocamlformat-0.26.0

39cd0df

Alizter reviewed Aug 1, 2023

View reviewed changes

Alizter approved these changes Aug 1, 2023

View reviewed changes

emillon merged commit 14d199f into ocaml:main Aug 1, 2023
19 checks passed

emillon mentioned this pull request Sep 26, 2023

Upgrade to OCamlformat 0.26.1 #8632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to ocamlformat 0.26.0 #8064

Upgrade to ocamlformat 0.26.0 #8064

Julow commented Jun 27, 2023

emillon commented Jun 28, 2023 •

edited

Loading

rgrinberg Jun 28, 2023

Julow Jun 28, 2023

rgrinberg Jun 28, 2023

Alizter Jun 28, 2023

Julow Jun 28, 2023

Julow Jul 5, 2023

emillon Jul 5, 2023

Julow Jul 7, 2023

Julow commented Jun 28, 2023

emillon commented Jul 5, 2023

Julow commented Jul 10, 2023

emillon Jul 10, 2023

Julow Jul 10, 2023

Julow Jul 17, 2023

emillon Jul 17, 2023

Julow Jul 17, 2023

emillon left a comment

Julow commented Jul 18, 2023

Julow commented Jul 20, 2023

emillon commented Jul 31, 2023

Alizter Aug 1, 2023

Julow Aug 1, 2023

Alizter left a comment

Upgrade to ocamlformat 0.26.0 #8064

Upgrade to ocamlformat 0.26.0 #8064

Conversation

Julow commented Jun 27, 2023

emillon commented Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julow commented Jun 28, 2023

emillon commented Jul 5, 2023

Julow commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emillon left a comment

Choose a reason for hiding this comment

Julow commented Jul 18, 2023

Julow commented Jul 20, 2023

emillon commented Jul 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alizter left a comment

Choose a reason for hiding this comment

emillon commented Jun 28, 2023 •

edited

Loading