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

Define a diff-friendly profile #2020

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Feb 22, 2022

Closes #1958

This is a new profile I wanted to work on for quite some time. It doesn't impact existing profiles.

New values are added to a few existing options, that users can set individually, although I advise to just use this profile if they want this behavior.

It also adds new fields to the Conf.fmt_opts type that should not become new options, this is just how we implement behavior switches in ocamlformat instead of matching on the profile.

  • unfold-let-binding : bool

@gpetiot gpetiot marked this pull request as ready for review February 23, 2022 11:06
@gpetiot gpetiot changed the title [WIP] Define a diff-friendly profile Define a diff-friendly profile Feb 23, 2022

type var =
[ `Foo (** foo *)
| `Bar of int * string (** bar *) ]
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this more diff-friendly:

type var = [
  | `Foo  (** foo *)
  | `Bar of int * string  (** bar *)
]

it would also be more cconsistent with records and regular variants

e.g. when adding or reordering cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it is, but the improvement of this behavior (controlled by dock-collection-brackets) have been greatly slowed down since it was introduced, a lot of cases are missing.

method m = 3
end
)
(* Invald_arg *)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be something like:

let _ =
  Obj.extension_constructor
    (
     object
       method m = 3
     end
    )

( module struct
let x =
false
end
Copy link
Contributor

Choose a reason for hiding this comment

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

the end does not match the module

if label <> lab then
raise VariantMismatch ;
set builder (devariantize field_type v)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this a place were exp_grouping= `Preserve could make more sense (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I meant letting users put begin .. end)

@tmattio
Copy link
Contributor

tmattio commented Mar 12, 2022

I just tried this on some code and really like it 🙂

Some notes:

-  val ( and+ )
-    :  'a Cmdliner.Term.t
-    -> 'b Cmdliner.Term.t
-    -> ('a * 'b) Cmdliner.Term.t
+  val ( and+ ) :
+    'a Cmdliner.Term.t -> 'b Cmdliner.Term.t -> ('a * 'b) Cmdliner.Term.t

The former seems more diff friendly?

-  Fmt_tty.setup_std_outputs ();
-  Logs.set_level log_level;
-  Logs.set_reporter (Logs_fmt.reporter ~app:Fmt.stdout ());
+  Fmt_tty.setup_std_outputs () ;
+  Logs.set_level log_level ;
+  Logs.set_reporter (Logs_fmt.reporter ~app:Fmt.stdout ()) 

Why add spaces there? It doesn't seem to improve the diff. I'm assuming this is something coming from the sparse profile, but should we use the default config here?

 let handle_errors = function
-  | Ok () -> if Logs.err_count () > 0 then 3 else 0
+  | Ok () ->
+      if Logs.err_count () > 0 then
+        3
+      else
+        0
   | Error err ->
-      Logs.err (fun m -> m "%s" (Error.to_string err));
+      Logs.err (fun m -> m "%s" (Error.to_string err)) ;
       error_to_code err

This does seem to be a regression, we don't need two level of indentation here I think.

 let term =
   let open Common.Syntax in
-  let+ _term = Common.term
+  let+ _term =
+    Common.term
   and+ name =
-    let doc = "The name to greet." in
-    let docv = "NAME" in
+    let doc =
+      "The name to greet."
+    in
+    let docv =
+      "NAME"
+    in

That indeed could improve the diff, but perhaps breaking every let binding into three lines is a bit much? 😅

My feedback aside, thanks a lot for working on this @gpetiot, I'm very happy to see a profile that uses objective guidelines as an alternative to the other profiles.

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.

Feature request: Update default profile to use indicate-multiline-delimiters = closing-on-separate-line
4 participants