Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to ocamlformat 0.26.0 #2262

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

Julow
Copy link
Contributor

@Julow Julow commented Jun 27, 2023

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

@Julow Julow marked this pull request as draft June 27, 2023 16:50
@art-w
Copy link
Contributor

art-w commented Jun 29, 2023

Thanks, the changes looks a lot better overall :)

@@ -25,7 +25,8 @@ module Server = struct
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -17,7 +17,8 @@
open! Import

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top!

@@ -0,0 +1,2 @@
# Upgrade to OCamlformat 0.26.0
e2488a04be2d19e5c826914028104c480a744e60
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -25,7 +25,8 @@ module Server = struct
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -17,7 +17,8 @@
open! Import

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😄

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

module Content_addressable (S : sig
include Irmin.Content_addressable.S
module Content_addressable
(S : sig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Julow
Copy link
Contributor Author

Julow commented Jul 17, 2023

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 Julow force-pushed the preview-ocamlformat-0.26.0 branch from f00ada6 to 62fe852 Compare July 18, 2023 09:33
Copy link
Contributor Author

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@metanivek
Copy link
Member

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

@Julow Julow force-pushed the preview-ocamlformat-0.26.0 branch from 62fe852 to 03fb9bd Compare July 20, 2023 10:21
@Julow Julow changed the title Preview: Upgrade to ocamlformat 0.26.0 (unreleased) Upgrade to ocamlformat 0.26.0 Jul 20, 2023
@Julow
Copy link
Contributor Author

Julow commented Jul 20, 2023

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 Julow marked this pull request as ready for review July 20, 2023 10:30
@metanivek metanivek merged commit 7065c66 into mirage:main Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants