Feature request: Don't wrap pipelines

[Julow]

The option break-infix=fit-or-vertical allows to break before every infix operators if the whole expression does not fit on a single line:

-           get_succs parent |> Sequence.of_list
+           get_succs parent
+           |> Sequence.of_list
            |> Sequence.filter ~f:(fun n -> not (equal node n))
            |> Sequence.Generator.of_sequence)

This works well for pipelines, boolean operators and sometimes list cons/concat but not with most other expressions:

 let git_clone ~branch ~remote ~output_dir =
   run_and_log
     Cmd.(
-      v "git" % "clone" % "--depth" % "1" % "--branch" % branch % remote
+      v "git"
+      % "clone"
+      % "--depth"
+      % "1"
+      % "--branch"
+      % branch
+      % remote
       % p output_dir)

This is clearly not what we want when the operands are short or when the expression is dense (eg. arithmetic, cmdliner's & and $).

I'd like fit-or-vertical in the default profile but only for pipelines.
Should we implement that ? How to decide what is a pipeline and what is not ?

There have been discussions before, in #69 then break-infix has been added in #150.

[gpetiot]

I agree, if we find a good heuristics we could also removes this option in favor of an auto mode:

• pipes -> fit or vertical
• others -> wrap

[jberdine]

I wonder if using the precedence of the operator would be a reasonable criterion. For example, perhaps everything at the precedence of :: and lower could be treated as fit-or-vertical, and everything of higher precedence could be treated as wrap.

[jberdine]

For maximal headaches, the threshold precedence level could be an int configure option. :-P

[craigfe]

Commenting my opinion on this suggestion as it came up in #1851. We use break-infix = fit-or-vertical in the Irmin code, and I think it's a benefit even in the non-pipeline cases. Here's a branch that applies the diff to drop the option, and some excerpts that I think are made worse:

let all_6_2char_hex a b c d e f =
-  is_2char_hex a
-  && is_2char_hex b
-  && is_2char_hex c
-  && is_2char_hex d
-  && is_2char_hex e
-  && is_2char_hex f
+  is_2char_hex a && is_2char_hex b && is_2char_hex c && is_2char_hex d
+  && is_2char_hex e && is_2char_hex f

  let pf0 =
    let open Utils.Parallel_folders in
-   open_ construct
-   |+ misc_stats_folder header
+   open_ construct |+ misc_stats_folder header
    |+ elapsed_wall_over_blocks_folder header block_count
    |+ elapsed_cpu_over_blocks_folder header block_count
    |+ Span_folder.create header.initial_stats.timestamp_wall block_count
    |+ cpu_usage_folder header block_count
-   |+ pack_folder
-   |+ tree_folder
-   |+ index_folder
-   |+ gc_folder
-   |+ disk_folder
+   |+ pack_folder |+ tree_folder |+ index_folder |+ gc_folder |+ disk_folder
+   |+ Store_watched_nodes_folder.create block_count
    |> seal

      let cmd =
        root
-       ^ "_build"
-         / "default"
+       ^ "_build" / "default"
          / Fmt.str "%s serve %d %d &" Sys.argv.(0) i id.id
      in

Overall, it's not clear to me why we should split e.g. large lists / chains of :: over multiple lines, but not chains of @. For the non-builtin cases, I suspect that it's not easy to predict how a user intends their DSL to be interpreted, and so the notion of what is a "pipeline" is very fluid. To take two examples:

• the operator ( >=> ) is sometimes used as a point-free equivalent of ( >>= ), but perhaps not commonly enough to make it worthwhile as an exception.
• operators like ( <+> ) are sometimes seen as an in-DSL equivalent of ( + ), and so it'd be nice to format them similarly.

(I don't even think that @Julow's original example is much worse when split onto multiple lines. Again, if I kept these arguments in a list, they would be formatted in the same way.) Overall, I think the formatting experience would be better (and much less surprising), with the existing fit-or-vertical behaviour.

CC: @samoht