-
Notifications
You must be signed in to change notification settings - Fork 157
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
Conversation
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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 -> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top!
2408768
to
e711c6d
Compare
.git-blame-ignore-revs
Outdated
@@ -0,0 +1,2 @@ | |||
# Upgrade to OCamlformat 0.26.0 | |||
e2488a04be2d19e5c826914028104c480a744e60 |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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)!
There was a problem hiding this 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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 -> |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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).
.git-blame-ignore-revs
Outdated
@@ -0,0 +1,2 @@ | |||
# Upgrade to OCamlformat 0.26.0 | |||
e2488a04be2d19e5c826914028104c480a744e60 |
There was a problem hiding this comment.
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)!
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 |
f00ada6
to
62fe852
Compare
There was a problem hiding this 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 ?
I'm good with this. Thanks again for the draft PR for feedback! |
62fe852
to
03fb9bd
Compare
OCamlformat 0.26.0 has been released, this PR can be merged (not squashed due to |
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