Skip to content

Commit

Permalink
Fix formatting of attributes on long lines (#86)
Browse files Browse the repository at this point in the history 
* add test of status quo

Signed-off-by: David Vulakh <[email protected]>

* add back break that was removed "for ocp compat"

Signed-off-by: David Vulakh <[email protected]>

* fix or-patterns

Signed-off-by: David Vulakh <[email protected]>

---------

Signed-off-by: David Vulakh <[email protected]>
  • Loading branch information
@dvulakh
dvulakh authored Nov 5, 2024
1 parent b3617c6 commit e40d7f4
Show file tree
Hide file tree
Showing 21 changed files with 333 additions and 110 deletions.
20 changes: 7 additions & 13 deletions lib/Fmt_ast.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -667,11 +667,7 @@ and fmt_attribute c ~key {attr_name; attr_payload; attr_loc} =
and fmt_attributes_aux c ?pre ?suf ~key attrs =
let num = List.length attrs in
fmt_if_k (num > 0)
( opt pre (function
(* Breaking before an attribute can confuse ocp-indent that will
produce a suboptimal indentation. *)
| Space when c.conf.fmt_opts.ocp_indent_compat.v -> sp Blank
| pre -> sp pre )
( opt pre sp
$ hvbox_if (num > 1) 0
(hvbox 0 (list attrs "@ " (fmt_attribute c ~key)) $ opt suf str) )

Expand Down Expand Up @@ -1242,9 +1238,7 @@ and fmt_pattern ?ext c ?pro ?parens ?(box = false)
@@ fun c ->
let parens = match parens with Some b -> b | None -> parenze_pat xpat in
(match ctx0 with Pat {ppat_desc= Ppat_tuple _; _} -> hvbox 0 | _ -> Fn.id)
@@ ( match ppat_desc with
| Ppat_or _ -> fun k -> Cmts.fmt c ppat_loc @@ k
| _ -> fun k -> Cmts.fmt c ppat_loc @@ (fmt_opt pro $ k) )
@@ (fun k -> Cmts.fmt c ppat_loc @@ (fmt_opt pro $ k))
@@ hovbox_if box 0
@@ fmt_pattern_attributes c xpat
@@
Expand Down Expand Up @@ -1433,7 +1427,8 @@ and fmt_pattern ?ext c ?pro ?parens ?(box = false)
| `Fit_or_vertical | `Vertical -> open_hvbox
| `Fit | `Nested | `Toplevel | `All -> open_hovbox
in
hvbox 0
hvbox
(if nested then -2 else 0)
( list_fl (List.group xpats ~break)
(fun ~first:first_grp ~last:_ xpat_grp ->
list_fl xpat_grp (fun ~first ~last:_ xpat ->
Expand All @@ -1450,10 +1445,9 @@ and fmt_pattern ?ext c ?pro ?parens ?(box = false)
in
let pro =
if first_grp && first then
fmt_opt pro
$ fits_breaks
(if parens then "(" else "")
(if nested then "" else "( ")
fits_breaks
(if parens then "(" else "")
(if nested then "" else "( ")
$ open_box (-2)
else if first then
Params.get_or_pattern_sep c.conf ~ctx:ctx0 ~cmts_before
Expand Down
36 changes: 36 additions & 0 deletions test/passing/dune.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,42 @@
(package ocamlformat)
(action (diff tests/attribute_and_expression.ml.js-err attribute_and_expression.ml.js-stderr)))

(rule
(deps tests/.ocamlformat )
(package ocamlformat)
(action
(with-stdout-to attribute_on_long_line.ml.stdout
(with-stderr-to attribute_on_long_line.ml.stderr
(run %{bin:ocamlformat} --margin-check %{dep:tests/attribute_on_long_line.ml})))))

(rule
(alias runtest)
(package ocamlformat)
(action (diff tests/attribute_on_long_line.ml.ref attribute_on_long_line.ml.stdout)))

(rule
(alias runtest)
(package ocamlformat)
(action (diff tests/attribute_on_long_line.ml.err attribute_on_long_line.ml.stderr)))

(rule
(deps tests/.ocamlformat )
(package ocamlformat)
(action
(with-stdout-to attribute_on_long_line.ml.js-stdout
(with-stderr-to attribute_on_long_line.ml.js-stderr
(run %{bin:ocamlformat} --profile=janestreet --enable-outside-detected-project --disable-conf-files %{dep:tests/attribute_on_long_line.ml})))))

(rule
(alias runtest)
(package ocamlformat)
(action (diff tests/attribute_on_long_line.ml.js-ref attribute_on_long_line.ml.js-stdout)))

(rule
(alias runtest)
(package ocamlformat)
(action (diff tests/attribute_on_long_line.ml.js-err attribute_on_long_line.ml.js-stderr)))

(rule
(deps tests/.ocamlformat )
(package ocamlformat)
Expand Down
37 changes: 37 additions & 0 deletions test/passing/tests/attribute_on_long_line.ml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
(* This is a separate test from [attributes.ml] because we want to be able to see the
output on the [janestreet] profile. *)

let _ =
very_long_function_name_that_causes_the_line_to_wrap_at_some_point [@alert
"-turn-it-off"]
;;

let () =
(f (fun () -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) [@aaaaaa
"This \
formatting \
is \
a \
bit \
strange"])
;;

let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
| (B | C _) [@alert "-foo"]
-> 0
| D -> 0
;;

(* Meanwhile, these are fine *)
let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | ((C _) [@alert "-foo"])
-> 0
| B | D -> 0
;;

let f = function
| (B | C _) [@alert "-foo"] | A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
-> 0
| D -> 0
;;
1 change: 1 addition & 0 deletions test/passing/tests/attribute_on_long_line.ml.err
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Warning: tests/attribute_on_long_line.ml:9 exceeds the margin
31 changes: 31 additions & 0 deletions test/passing/tests/attribute_on_long_line.ml.js-ref
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
(* This is a separate test from [attributes.ml] because we want to be able to see the
output on the [janestreet] profile. *)

let _ =
very_long_function_name_that_causes_the_line_to_wrap_at_some_point
[@alert "-turn-it-off"]
;;

let () =
(f (fun () -> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa)
[@aaaaaa "This formatting is a bit strange"])
;;

let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
| (B | C _) [@alert "-foo"] -> 0
| D -> 0
;;

(* Meanwhile, these are fine *)
let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) | ((C _) [@alert "-foo"])
-> 0
| B | D -> 0
;;

let f = function
| (B | C _) [@alert "-foo"] | A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
-> 0
| D -> 0
;;
30 changes: 30 additions & 0 deletions test/passing/tests/attribute_on_long_line.ml.ref
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
(* This is a separate test from [attributes.ml] because we want to be able to
see the output on the [janestreet] profile. *)

let _ =
very_long_function_name_that_causes_the_line_to_wrap_at_some_point
[@alert "-turn-it-off"]

let () =
(f (fun () ->
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa )
[@aaaaaa "This formatting is a bit strange"] )

let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
|(B | C _) [@alert "-foo"] ->
0
| D -> 0

(* Meanwhile, these are fine *)
let f = function
| A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
|((C _) [@alert "-foo"]) ->
0
| B | D -> 0

let f = function
| (B | C _) [@alert "-foo"]
|A (0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) ->
0
| D -> 0
15 changes: 10 additions & 5 deletions test/passing/tests/break_collection_expressions-wrap.ml.js-ref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
|] [@foo]
|]
[@foo]
;;

let length =
Expand All @@ -71,7 +72,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
:] [@foo]
:]
[@foo]
;;

let length =
Expand Down Expand Up @@ -173,7 +175,8 @@ let length =
; 27
; 27
; 28
|] [@foo]
|]
[@foo]
;;

let length =
Expand Down Expand Up @@ -271,7 +274,8 @@ let length =
; 27
; 27
; 28
:] [@foo]
:]
[@foo]
;;

let length =
Expand Down Expand Up @@ -388,5 +392,6 @@ let length =
; 27
; 27
; 28
] [@foo]
]
[@foo]
;;
15 changes: 10 additions & 5 deletions test/passing/tests/break_collection_expressions.ml.js-ref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
|] [@foo]
|]
[@foo]
;;

let length =
Expand All @@ -71,7 +72,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
:] [@foo]
:]
[@foo]
;;

let length =
Expand Down Expand Up @@ -173,7 +175,8 @@ let length =
; 27
; 27
; 28
|] [@foo]
|]
[@foo]
;;

let length =
Expand Down Expand Up @@ -271,7 +274,8 @@ let length =
; 27
; 27
; 28
:] [@foo]
:]
[@foo]
;;

let length =
Expand Down Expand Up @@ -388,5 +392,6 @@ let length =
; 27
; 27
; 28
] [@foo]
]
[@foo]
;;
15 changes: 10 additions & 5 deletions test/passing/tests/break_separators-after.ml.js-ref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
|] [@foo]
|]
[@foo]
;;

(* this is an immutable array *)
Expand All @@ -175,7 +176,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
:] [@foo]
:]
[@foo]
;;

(* this is a list *)
Expand All @@ -189,7 +191,8 @@ let length =
(* this is a list comprehension *)
let pythagorean =
[ a, b, c for a = 1 to 10 for b = a to 10 for c = b to 10 when (a * a) + (b * b) = c * c
] [@foo]
]
[@foo]
;;

(* this is an array comprehension *)
Expand All @@ -199,7 +202,8 @@ let pythagorean =
for b = a to 10
for c = b to 10
when (a * a) + (b * b) = c * c
|] [@foo]
|]
[@foo]
;;

(* this is an immutable array comprehension *)
Expand All @@ -209,7 +213,8 @@ let pythagorean =
for b = a to 10
for c = b to 10
when (a * a) + (b * b) = c * c
:] [@foo]
:]
[@foo]
;;

Fooooooo.foo
Expand Down
15 changes: 10 additions & 5 deletions test/passing/tests/break_separators-after_docked.ml.js-ref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
|] [@foo]
|]
[@foo]
;;

(* this is an immutable array *)
Expand All @@ -175,7 +176,8 @@ let length =
; (* foo *) 27 (* foo *)
; 27
; 27
:] [@foo]
:]
[@foo]
;;

(* this is a list *)
Expand All @@ -189,7 +191,8 @@ let length =
(* this is a list comprehension *)
let pythagorean =
[ a, b, c for a = 1 to 10 for b = a to 10 for c = b to 10 when (a * a) + (b * b) = c * c
] [@foo]
]
[@foo]
;;

(* this is an array comprehension *)
Expand All @@ -199,7 +202,8 @@ let pythagorean =
for b = a to 10
for c = b to 10
when (a * a) + (b * b) = c * c
|] [@foo]
|]
[@foo]
;;

(* this is an immutable array comprehension *)
Expand All @@ -209,7 +213,8 @@ let pythagorean =
for b = a to 10
for c = b to 10
when (a * a) + (b * b) = c * c
:] [@foo]
:]
[@foo]
;;

Fooooooo.foo
Expand Down
Loading

0 comments on commit e40d7f4

Please sign in to comment.