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.

Sign up for GitHub

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
Merged

Upgrade to ocamlformat 0.26.0 #2262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
faab08a
Upgrade to OCamlformat 0.26.0
Julow Jul 20, 2023
03fb9bd
Update .git-blame-ignore-revs
Julow Jul 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .git-blame-ignore-revs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# Upgrade to OCamlformat 0.26.0
faab08a7dd9c275111cae51651a78fd62ac3e031
2 changes: 1 addition & 1 deletion .ocamlformat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version = 0.24.1
version = 0.26.0
profile = conventional

ocaml-version = 4.08.0
Expand Down
4 changes: 2 additions & 2 deletions src/irmin-chunk/irmin_chunk.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ struct
in
let chunking = Conf.get config Conf.Key.chunking in
(if max_children <= 1 then
let min = Chunk.size_of_index_header + (H.hash_size * 2) in
err_too_small ~min chunk_size);
let min = Chunk.size_of_index_header + (H.hash_size * 2) in
err_too_small ~min chunk_size);
[%log.debug
"config: chunk-size=%d digest-size=%d max-data=%d max-children=%d"
chunk_size H.hash_size max_data max_children];
Expand Down
6 changes: 4 additions & 2 deletions src/irmin-graphql/unix/irmin_graphql_unix.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

(T : Irmin_graphql.Server.CUSTOM_TYPES
Expand Down Expand Up @@ -55,7 +56,8 @@ module Server = struct
(T)

module Make
(S : Irmin.Generic_key.S) (Remote : sig
(S : Irmin.Generic_key.S)
(Remote : sig
val remote : remote_fn option
end) =
Irmin_graphql.Server.Make
Expand Down
6 changes: 4 additions & 2 deletions src/irmin-graphql/unix/irmin_graphql_unix.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ module Server : sig
end

module Make
(S : Irmin.Generic_key.S) (Remote : sig
(S : Irmin.Generic_key.S)
(Remote : sig
val remote : remote_fn option
end) :
Irmin_graphql.Server.S
Expand All @@ -34,7 +35,8 @@ module Server : sig
and module IO = Cohttp_lwt_unix.IO

module Make_ext
(S : Irmin.Generic_key.S) (Remote : sig
(S : Irmin.Generic_key.S)
(Remote : sig
val remote : remote_fn option
end)
(T : Irmin_graphql.Server.CUSTOM_TYPES
Expand Down
13 changes: 7 additions & 6 deletions src/irmin-http/irmin_http_server.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ module Make (HTTP : Cohttp_lwt.S.Server) (S : Irmin.S) = struct
let err = Fmt.str "Parse error %S: %s" str e in
Wm.respond ~body:(`String err) 400 rd

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

include Irmin.Content_addressable.S

val batch : B.Repo.t -> (read_write t -> 'a Lwt.t) -> 'a Lwt.t
end)
(K : Irmin.Type.S with type t = S.key)
(V : Irmin.Type.S with type t = S.value) =
val batch : B.Repo.t -> (read_write t -> 'a Lwt.t) -> 'a Lwt.t
end)
(K : Irmin.Type.S with type t = S.key)
(V : Irmin.Type.S with type t = S.value) =
struct
let with_key rd f =
match Irmin.Type.of_string K.t (Wm.Rd.lookup_path_info_exn "id" rd) with
Expand Down
3 changes: 2 additions & 1 deletion src/irmin-pack/inode.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ exception Max_depth of int

module Make_internal
(Conf : Conf.S)
(H : Irmin.Hash.S) (Key : sig
(H : Irmin.Hash.S)
(Key : sig
include Irmin.Key.S with type hash = H.t

val unfindable_of_hash : hash -> t
Expand Down
3 changes: 2 additions & 1 deletion src/irmin-pack/inode_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,8 @@ module type Sigs = sig

module Make_internal
(Conf : Conf.S)
(H : Irmin.Hash.S) (Key : sig
(H : Irmin.Hash.S)
(Key : sig
include Irmin.Key.S with type hash = H.t

val unfindable_of_hash : hash -> t
Expand Down
3 changes: 2 additions & 1 deletion src/irmin-pack/unix/gc.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

module Args : Gc_args.S

type t
Expand Down
3 changes: 2 additions & 1 deletion src/irmin-pack/unix/gc_worker.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
open! Import
module Payload = Control_file.Payload.Upper.Latest

module Make (Args : Gc_args.S) : sig
module Make
(Args : Gc_args.S) : sig
module Args : Gc_args.S

val run_and_output_result :
Expand Down
26 changes: 13 additions & 13 deletions src/irmin-pack/unix/lower.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,19 +385,19 @@ module Make (Io : Io.S) (Errs : Io_errors.S with module Io = Io) = struct
let archive_seq_exn ~upper_root ~generation ~to_archive t =
Errs.raise_if_error
(let open Result_syntax in
let* () = if t.readonly then Error `Ro_not_allowed else Ok () in
let* v =
match appendable_volume t with
| None -> Error `Lower_has_no_volume
| Some v -> Ok v
in
let* () =
match t.open_volume with
| None -> Ok ()
| Some v0 -> if Volume.eq v0 v then close_open_volume t else Ok ()
in
let is_first = volume_num t = 1 in
Volume.archive_seq ~upper_root ~generation ~to_archive ~is_first v)
let* () = if t.readonly then Error `Ro_not_allowed else Ok () in
let* v =
match appendable_volume t with
| None -> Error `Lower_has_no_volume
| Some v -> Ok v
in
let* () =
match t.open_volume with
| None -> Ok ()
| Some v0 -> if Volume.eq v0 v then close_open_volume t else Ok ()
in
let is_first = volume_num t = 1 in
Volume.archive_seq ~upper_root ~generation ~to_archive ~is_first v)

let read_exn ~off ~len ?volume t b =
let _, volume = read_range_exn ~off ~min_len:len ~max_len:len ?volume t b in
Expand Down
6 changes: 3 additions & 3 deletions src/irmin-pack/unix/pack_store.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,9 +232,9 @@ struct
become valid on [reload]; otherwise we know that this key wasn't
constructed for this store. *)
(if not (Control.readonly (Fm.control t.fm)) then
let io_offset = Dispatcher.end_offset t.dispatcher in
invalid_read "invalid key %a checked for membership (IO offset = %a)"
pp_key k Int63.pp io_offset);
let io_offset = Dispatcher.end_offset t.dispatcher in
invalid_read "invalid key %a checked for membership (IO offset = %a)"
pp_key k Int63.pp io_offset);
false
| Errors.Pack_error (`Invalid_sparse_read _) -> false
| Errors.Pack_error (`Invalid_prefix_read _) -> false
Expand Down
3 changes: 2 additions & 1 deletion src/irmin-test/irmin_bench.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*)

module Make (S : Irmin.Generic_key.KV with type Schema.Contents.t = string) : sig
module Make
(S : Irmin.Generic_key.KV with type Schema.Contents.t = string) : sig
val run :
config:(root:string -> Irmin.config) -> size:(root:string -> int) -> unit
end
4 changes: 3 additions & 1 deletion src/irmin/commit_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,9 @@ module type Sigs = sig
module Info : Info.S with type t = Info.Default.t
(** Serialisation format for V1 info. *)

module Make (Hash : Hash.S) (C : Generic_key.S with module Info := Info) : sig
module Make
(Hash : Hash.S)
(C : Generic_key.S with module Info := Info) : sig
include
Generic_key.S
with module Info = Info
Expand Down
4 changes: 2 additions & 2 deletions src/irmin/dot.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ module Make (S : Store.Generic_key.S) = struct
let label_of_node k _ =
let s =
(if html then
sprintf "<div class='node'><div class='sha1'>%s</div></div>"
else fun x -> x)
sprintf "<div class='node'><div class='sha1'>%s</div></div>"
else fun x -> x)
(string_of_hash Node.Hash.t k)
in
`Label s
Expand Down
4 changes: 2 additions & 2 deletions src/irmin/irmin.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ end
module Type = Repr
(** Dynamic types for Irmin values, supplied by
{{:https://github.com/mirage/repr} [Repr]}. These values can be derived from
type definitions via [\[@@deriving irmin\]] (see the
type definitions via [[@@deriving irmin]] (see the
{{:https://github.com/mirage/irmin/blob/main/README_PPX.md} documentation
for [ppx_irmin]})*)
for [ppx_irmin]})*)

module Hash = Hash
(** Hashing functions.
Expand Down
12 changes: 8 additions & 4 deletions src/irmin/node.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ module Irmin_hash = Hash
(* A [Make] implementation providing the subset of [S] that can be implemented
over abstract [key] types. *)
module Make_core
(Hash : Hash.S) (Path : sig
(Hash : Hash.S)
(Path : sig
type step [@@deriving irmin]
end)
(Metadata : Metadata.S)
Expand Down Expand Up @@ -335,7 +336,8 @@ module Portable = struct
end

module Make_generic_key
(Hash : Hash.S) (Path : sig
(Hash : Hash.S)
(Path : sig
type step [@@deriving irmin]
end)
(Metadata : Metadata.S)
Expand Down Expand Up @@ -403,7 +405,8 @@ struct
end

module Make_generic_key_v2
(Hash : Hash.S) (Path : sig
(Hash : Hash.S)
(Path : sig
type step [@@deriving irmin]
end)
(Metadata : Metadata.S)
Expand All @@ -422,7 +425,8 @@ struct
end

module Make
(Hash : Hash.S) (Path : sig
(Hash : Hash.S)
(Path : sig
type step [@@deriving irmin]
end)
(Metadata : Metadata.S) =
Expand Down
3 changes: 2 additions & 1 deletion src/irmin/node_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,8 @@ module type Sigs = sig
and metadata implementations. The contents and node values are addressed
directly by their hash. *)
module Make
(Hash : Hash.S) (Path : sig
(Hash : Hash.S)
(Path : sig
type step [@@deriving irmin]
end)
(Metadata : Metadata.S) :
Expand Down
3 changes: 2 additions & 1 deletion src/irmin/proof.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ include Proof_intf

module Make
(C : Type.S)
(H : Type.S) (S : sig
(H : Type.S)
(S : sig
type step [@@deriving irmin]
end)
(M : Type.S) =
Expand Down
9 changes: 5 additions & 4 deletions src/irmin/proof_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ module type S = sig
concrete proof or a hash of that proof.

If an inode proof contains singleton children [i_0, ..., i_n] such as:
[{length=l; proofs = \[ (i_0, {proofs = ... { proofs = \[ (i_n, p) \] }})\]}],
[{length=l; proofs = [ (i_0, {proofs = ... { proofs = [ (i_n, p) ] }})]}],
then it is compressed into the inode extender
[{length=l; segment = \[i_0;..;i_n\]; proof=p}] sharing the same length
[l] and final proof [p]. *)
[{length=l; segment = [i_0;..;i_n]; proof=p}] sharing the same length [l]
and final proof [p]. *)

(** The type for compressed and partial Merkle tree proofs.

Expand Down Expand Up @@ -329,7 +329,8 @@ module type Proof = sig

module Make
(C : Type.S)
(H : Hash.S) (P : sig
(H : Hash.S)
(P : sig
type step [@@deriving irmin]
end)
(M : Type.S) : sig
Expand Down
2 changes: 1 addition & 1 deletion src/irmin/store_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,7 @@ module type S_generic_key = sig
(** {Deprecated} *)

val master : repo -> t Lwt.t
[@@ocaml.deprecated "Use `main` instead."]
[@@ocaml.deprecated "Use `main` instead."]
(** @deprecated Use {!main} instead *)
end

Expand Down
4 changes: 2 additions & 2 deletions src/irmin/tree.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -921,7 +921,7 @@ module Make (P : Backend.S) = struct
let rec aux acc = function
| [] ->
(if cache then
match acc with Node n -> t.info.value <- Some n | Pnode _ -> ());
match acc with Node n -> t.info.value <- Some n | Pnode _ -> ());
k acc
| (k, Add e) :: rest ->
hash_preimage_value_of_elt ~cache e (fun e ->
Expand Down Expand Up @@ -2420,7 +2420,7 @@ module Make (P : Backend.S) = struct
| [] ->
k
(if StepMap.is_empty map then Empty
else Non_empty (Node.of_map ~env:(Env.empty ()) map))
else Non_empty (Node.of_map ~env:(Env.empty ()) map))
| (s, n) :: t ->
(concrete [@tailcall]) n (fun v ->
(tree [@tailcall])
Expand Down
5 changes: 3 additions & 2 deletions src/irmin/tree_intf.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,9 @@ module type S = sig
[post path steps].
- If [n] is any kind, call [tree path t'] where [t'] is the tree of [n].

See {{:https://github.com/mirage/irmin/blob/main/examples/fold.ml}
examples/fold.ml} for a demo of the different {!folder}s.
See
{{:https://github.com/mirage/irmin/blob/main/examples/fold.ml}
examples/fold.ml} for a demo of the different {!folder}s.

See {!force} for details about the [force] parameters. By default it is
[`True].
Expand Down
20 changes: 10 additions & 10 deletions src/libirmin/commit.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ module Make (I : Cstubs_inverted.INTERNAL) = struct
(repo @-> hash @-> returning commit)
(fun (type repo) repo hash ->
with_repo' repo commit
(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 hash = Root.get_hash (module Store) hash in
let c = run (Store.Commit.of_hash repo hash) in
match c with
Expand All @@ -62,8 +62,8 @@ module Make (I : Cstubs_inverted.INTERNAL) = struct
(repo @-> commit_key @-> returning commit)
(fun (type repo) repo hash ->
with_repo' repo commit
(fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
->
(fun
(module Store : Irmin.Generic_key.S with type repo = repo) repo ->
let hash = Root.get_commit_key (module Store) hash in
let c = run (Store.Commit.of_key repo hash) in
match c with
Expand All @@ -75,8 +75,8 @@ module Make (I : Cstubs_inverted.INTERNAL) = struct
(repo @-> ptr commit @-> uint64_t @-> tree @-> info @-> returning commit)
(fun (type repo) repo parents n tree info ->
with_repo' repo commit
(fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
->
(fun
(module Store : Irmin.Generic_key.S with type repo = repo) repo ->
let n = UInt64.to_int n in
let parents =
if is_null parents || n = 0 then []
Expand All @@ -97,8 +97,8 @@ module Make (I : Cstubs_inverted.INTERNAL) = struct
(repo @-> commit @-> returning commit_array)
(fun (type repo) repo commit ->
with_repo' repo commit_array
(fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
->
(fun
(module Store : Irmin.Generic_key.S with type repo = repo) repo ->
let commit = Root.get_commit (module Store) commit in
let parents = Store.Commit.parents commit in
let parents =
Expand All @@ -117,8 +117,8 @@ module Make (I : Cstubs_inverted.INTERNAL) = struct
(repo @-> commit @-> commit @-> returning bool)
(fun (type repo) repo a b ->
with_repo repo false
(fun (module Store : Irmin.Generic_key.S with type repo = repo) repo
->
(fun
(module Store : Irmin.Generic_key.S with type repo = repo) repo ->
let a = Root.get_commit (module Store) a in
let b = Root.get_commit (module Store) b in
Irmin.Type.(unstage (equal (Store.commit_t repo))) a b))
Expand Down
Loading