Upgrade to ocamlformat 0.26.0 #2262

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

The formatting of module arguments changed and this cause large diffs on Irmin.

The aim of this commit is to gather feedback.

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

art-w · 2023-06-29T09:22:41Z

Thanks, the changes looks a lot better overall :)

art-w · 2023-06-29T09:26:53Z

src/irmin-graphql/unix/irmin_graphql_unix.ml

+      (S : Irmin.Generic_key.S)
+      (Remote : sig
        val remote : remote_fn option
      end)


This is great!

Nitpick questions: It looks like theRemote : sig...end could fit on a single line? The end is missing an indentation space to indicate that it is within the opening parens?

Indenting the end one more means indenting the body of the sig one more too ? This would cause a huge diff.

Indenting the end but not the body is not so bad:

module Make_ext (S : Irmin.Generic_key.S) (Remote : sig val remote : remote_fn option end)

I would like the opinion of more people on this :)

Personally, I think both look sort of strange, but I don't care too much either way. Consistency is more important to me. Are there other scenarios of formatting that enforce either choice? Looking at some of the other formatting changes it seems like perhaps the most consistent is to indent everything by a single space, not just the end.

Nested things are indented 2 + width of the parens, for example in a if.

I'm not against indenting the end but without a strong opinion, I'm tempted to leave it like that.

Would it be more consistent with the "2 space + width of paren" rule to indent this like the following by adding 1 space (width of paren) to both val and end -- essentially format them relative to Remote instead of (Remote? The current formatting does not seem to take the paren into consideration (unlike the diff I linked in my prior comment), but maybe in this context of functor arguments the intention is for the opening paren to be considered the first column of the block (?).

module Make_ext (S : Irmin.Generic_key.S) (Remote : sig val remote : remote_fn option end)

That would be ideal but would cause huge diffs to existing code.

I plan to make more changes to module types after the release, huge changes like that could be accepted in combination of other already large changes.

art-w · 2023-06-29T09:29:45Z

src/irmin-pack/unix/gc.mli

 (** [Make] returns a module that can manage GC processes. *)
-module Make (Args : Gc_args.S) : sig
+module Make
+    (Args : Gc_args.S) : sig


This is worse :P (but much less important than the corrected functor argument layout)

I have spent a bit of time on this issue and I could only fix it as part of a large refactoring, that is not ready to be released (ocaml-ppx/ocamlformat#2395)

How bad would it be to roll the release with this new bug ?

It's not the end of the world but I agree with @art-w that it would be nice to fix. We can also wait for the fix if it's upcoming. 😄

Skipping a version is fine. Though, that's bigger diffs for next time.

art-w · 2023-06-29T09:32:16Z

src/libirmin/commit.ml

-          (fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
-          ->
+          (fun
+            (module Store : Irmin.Generic_key.S with type repo = repo) repo ->


Nitpick: I don't think I like breaking after the fun keyword because it creates weird alignments between the arguments and the body, but the original code is problematic to indent nicely anyway (so it doesn't matter much)

The fun has to break if the arguments are even bigger and the arrow is better there to avoid a line with only the arrow:

(fun (module Store : Irmin.Generic_key.S with type repo = repo) (module Store : Irmin.Generic_key.S with type repo = repo) -> body)

I agree with weird alignment. More indentation ?

(fun (module Store : Irmin.Generic_key.S with type repo = repo) (module Store : Irmin.Generic_key.S with type repo = repo) -> body)

I think indenting the arguments would be consistent with the functor argument formatting.

Agree! Though, that's not possible without huge diffs. I would like to discuss this with more people here: ocaml-ppx/ocamlformat#2397

art-w · 2023-06-29T09:32:57Z

test/irmin-pack/test_inode.ml

-      let module Inter = (val inter : Irmin_pack.Inode.Internal
-                            with type Val.step = Path.step)
+      let module Inter =
+        (val inter : Irmin_pack.Inode.Internal with type Val.step = Path.step)


Julow · 2023-07-10T10:28:31Z

.git-blame-ignore-revs

@@ -0,0 +1,2 @@
+# Upgrade to OCamlformat 0.26.0
+e2488a04be2d19e5c826914028104c480a744e60


This file has helped me in the past. It's used with git blame --ignore-revs-file=.git-blame-ignore-revs.
Is this a welcome addition ?

I've never seen this before but seems useful (and automatically supported by github)!

metanivek

Lots of improvements here! Left some thoughts that are (hopefully) helpful/constructive. Thanks for opening this to gather feedback and sorry for the delay!

metanivek · 2023-07-14T19:42:10Z

src/irmin-graphql/unix/irmin_graphql_unix.ml

+      (S : Irmin.Generic_key.S)
+      (Remote : sig
        val remote : remote_fn option
      end)


Personally, I think both look sort of strange, but I don't care too much either way. Consistency is more important to me. Are there other scenarios of formatting that enforce either choice? Looking at some of the other formatting changes it seems like perhaps the most consistent is to indent everything by a single space, not just the end.

metanivek · 2023-07-14T19:45:48Z

src/irmin-pack/unix/gc.mli

 (** [Make] returns a module that can manage GC processes. *)
-module Make (Args : Gc_args.S) : sig
+module Make
+    (Args : Gc_args.S) : sig


It's not the end of the world but I agree with @art-w that it would be nice to fix. We can also wait for the fix if it's upcoming. 😄

metanivek · 2023-07-14T19:54:19Z

src/libirmin/commit.ml

-          (fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
-          ->
+          (fun
+            (module Store : Irmin.Generic_key.S with type repo = repo) repo ->


I think indenting the arguments would be consistent with the functor argument formatting.

metanivek · 2023-07-14T19:58:32Z

src/irmin-http/irmin_http_server.ml

-  module Content_addressable (S : sig
-    include Irmin.Content_addressable.S
+  module Content_addressable
+      (S : sig


Why the two additional spaces? It seems like the general rhythm is 2 spaces per indentation. Why should the arguments be indented twice under module (versus the original 1 for the include line).

metanivek · 2023-07-14T20:01:56Z

.git-blame-ignore-revs

@@ -0,0 +1,2 @@
+# Upgrade to OCamlformat 0.26.0
+e2488a04be2d19e5c826914028104c480a744e60


I've never seen this before but seems useful (and automatically supported by github)!

Julow · 2023-07-17T08:17:18Z

Why the two additional spaces? It seems like the general rhythm is 2 spaces per indentation. Why should the arguments be indented twice under module (versus the original 1 for the include line).

This is the existing formatting for module arguments so changing it would cause huge diffs. A bigger indentation is used to disambiguate the arguments and the body.

The previous form that looked like that is a special case and still exists. The change is that it applies to modules with one arguments only:

  module Content_addressable (S : sig
    include Irmin.Content_addressable.S
  end) =
  struct

Julow

Although a small bug remains on module types, I'm planning to release soon. Is this good with everyone ?

metanivek · 2023-07-18T16:29:23Z

I'm good with this. Thanks again for the draft PR for feedback!

Julow · 2023-07-20T10:30:01Z

OCamlformat 0.26.0 has been released, this PR can be merged (not squashed due to .git-blame-ignore-revs) or closed if it's a bit too early to upgrade.

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

art-w reviewed Jun 29, 2023

View reviewed changes

Julow mentioned this pull request Jul 6, 2023

Consistent formatting of module types ocaml-ppx/ocamlformat#2395

Open

Julow force-pushed the preview-ocamlformat-0.26.0 branch from 2408768 to e711c6d Compare July 10, 2023 10:12

Julow commented Jul 10, 2023

View reviewed changes

metanivek reviewed Jul 14, 2023

View reviewed changes

Julow force-pushed the preview-ocamlformat-0.26.0 branch from f00ada6 to 62fe852 Compare July 18, 2023 09:33

Julow commented Jul 18, 2023

View reviewed changes

Julow added 2 commits July 20, 2023 12:21

Upgrade to OCamlformat 0.26.0

faab08a

Update .git-blame-ignore-revs

03fb9bd

Julow force-pushed the preview-ocamlformat-0.26.0 branch from 62fe852 to 03fb9bd Compare July 20, 2023 10:21

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:30

metanivek merged commit 7065c66 into mirage:main Aug 1, 2023

		@@ -0,0 +1,2 @@
		# Upgrade to OCamlformat 0.26.0
		e2488a04be2d19e5c826914028104c480a744e60

Upgrade to ocamlformat 0.26.0 #2262

Upgrade to ocamlformat 0.26.0 #2262

Uh oh!

Conversation

Julow commented Jun 27, 2023

Uh oh!

art-w commented Jun 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metanivek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julow commented Jul 17, 2023

Uh oh!

Julow left a comment

Choose a reason for hiding this comment

Uh oh!

metanivek commented Jul 18, 2023

Uh oh!

Julow commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants