Skip to content

Commit c0660a4

Browse files
jberdinejberdine
authored
Improve breaking of comments to avoid violating the margin (ocaml-ppx#1676)
1 parent ec0da1d commit c0660a4

32 files changed

+111
-85
lines changed

Diff for: ‎CHANGES.md

+2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030

3131
+ Remove double parens around a functor in a module application (#1681, @gpetiot)
3232

33+
+ Improve breaking of comments to avoid violating the margin (#1676, @jberdine)
34+
3335
#### Changes
3436

3537
+ Improve the diff of unstable docstrings displayed in error messages (#1654, @gpetiot)

Diff for: ‎lib/Ast.ml

+2-1
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,8 @@ module Indexing_op = struct
163163
| Some (lhs :: args) -> (
164164
match ident with
165165
| {pexp_desc= Pexp_ident {txt= ident; loc}; pexp_attributes= []; _}
166-
(* We only use the sugared form if it was already used in the source. *)
166+
(* We only use the sugared form if it was already used in the
167+
source. *)
167168
when loc.loc_ghost -> (
168169
match get_sugar_ident ident args with
169170
| None -> None

Diff for: ‎lib/Cmt.ml

+2-2
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,8 @@ let fmt cmt src ~wrap:wrap_comments ~ocp_indent_compat ~fmt_code pos =
147147
| [] -> assert false
148148
| [""] -> assert false
149149
| [""; ""] -> str "(* *)"
150-
| [text] -> str "(*" $ fill_text text ~epi:(str "*)")
151-
| [text; ""] -> str "(*" $ fill_text text ~epi:(str " *)")
150+
| [text] -> str "(*" $ fill_text text ~epi:"*)"
151+
| [text; ""] -> str "(*" $ fill_text text ~epi:" *)"
152152
| asterisk_prefixed_lines ->
153153
fmt_asterisk_prefixed_lines asterisk_prefixed_lines
154154
in

Diff for: ‎lib/Conf.ml

+2-1
Original file line numberDiff line numberDiff line change
@@ -1125,7 +1125,8 @@ module Formatting = struct
11251125
(fun conf -> conf.wrap_fun_args)
11261126
end
11271127

1128-
(* Flags that can be modified in the config file that don't affect formatting *)
1128+
(* Flags that can be modified in the config file that don't affect
1129+
formatting *)
11291130

11301131
let project_root_witness = [".git"; ".hg"; "dune-project"]
11311132

Diff for: ‎lib/Fmt.ml

+22-20
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,9 @@ let char c = with_pp (fun fs -> Format.pp_print_char fs c)
121121
let utf8_length s =
122122
Uuseg_string.fold_utf_8 `Grapheme_cluster (fun n _ -> n + 1) 0 s
123123

124-
let str s =
125-
with_pp (fun fs ->
126-
if not (String.is_empty s) then Format.pp_print_as fs (utf8_length s) s )
124+
let str_as n s = with_pp (fun fs -> Format.pp_print_as fs n s)
125+
126+
let str s = if String.is_empty s then noop else str_as (utf8_length s) s
127127

128128
let sp = function
129129
| Blank -> char ' '
@@ -322,7 +322,7 @@ and hovbox_if ?name cnd n = wrap_if_k cnd (open_hovbox ?name n) close_box
322322

323323
(** Text filling --------------------------------------------------------*)
324324

325-
let fill_text ?epi text =
325+
let fill_text ?(epi = "") text =
326326
assert (not (String.is_empty text)) ;
327327
let fmt_line line =
328328
let words =
@@ -337,19 +337,21 @@ let fill_text ?epi text =
337337
~equal:(fun x y -> String.is_empty x && String.is_empty y)
338338
(String.split (String.rstrip text) ~on:'\n')
339339
in
340-
fmt_if (String.starts_with_whitespace text) " "
341-
$ hovbox 0
342-
( hvbox 0
343-
(hovbox 0
344-
(list_pn lines (fun ~prev:_ curr ~next ->
345-
fmt_line curr
346-
$
347-
match next with
348-
| Some str when String.for_all str ~f:Char.is_whitespace ->
349-
close_box $ fmt "\n@," $ open_hovbox 0
350-
| Some _ when not (String.is_empty curr) -> fmt "@ "
351-
| _ -> noop ) ) )
352-
$ fmt_if
353-
(String.length text > 1 && String.ends_with_whitespace text)
354-
" "
355-
$ opt epi Fn.id )
340+
let pro = if String.starts_with_whitespace text then " " else "" in
341+
let epi =
342+
if String.length text > 1 && String.ends_with_whitespace text then
343+
" " ^ epi
344+
else epi
345+
in
346+
str pro
347+
$ hvbox 0
348+
(hovbox 0
349+
( list_pn lines (fun ~prev:_ curr ~next ->
350+
fmt_line curr
351+
$
352+
match next with
353+
| Some str when String.for_all str ~f:Char.is_whitespace ->
354+
close_box $ fmt "\n@," $ open_hovbox 0
355+
| Some _ when not (String.is_empty curr) -> fmt "@ "
356+
| _ -> noop )
357+
$ str epi ) )

Diff for: ‎lib/Fmt.mli

+1-1
Original file line numberDiff line numberDiff line change
@@ -227,5 +227,5 @@ val hovbox_if : ?name:string -> bool -> int -> t -> t
227227

228228
(** Text filling --------------------------------------------------------*)
229229

230-
val fill_text : ?epi:t -> string -> t
230+
val fill_text : ?epi:string -> string -> t
231231
(** Format a non-empty string as filled text wrapped at the margin. *)

Diff for: ‎lib/Fmt_ast.ml

+6-3
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,11 @@ module Cmts = struct
3838
let fmt c ?pro ?epi ?eol ?adj loc =
3939
(* remove the before comments from the map first *)
4040
let before = fmt_before c ?pro ?epi ?eol ?adj loc in
41-
(* remove the within comments from the map by accepting the continuation *)
41+
(* remove the within comments from the map by accepting the
42+
continuation *)
4243
fun inner ->
43-
(* delay the after comments until the within comments have been removed *)
44+
(* delay the after comments until the within comments have been
45+
removed *)
4446
let after = fmt_after c ?pro ?epi loc in
4547
let open Fmt in
4648
before $ inner $ after
@@ -1809,7 +1811,8 @@ and fmt_expression c ?(box = true) ?pro ?epi ?eol ?parens ?(indent_wrap = 0)
18091811
when List.for_all rev_e1N ~f:(fun (_, eI) ->
18101812
is_simple c.conf (fun _ -> 0) (sub_exp ~ctx eI) ) ->
18111813
let e1N = List.rev rev_e1N in
1812-
(* side effects of Cmts.fmt c.cmts before Sugar.fun_ is important *)
1814+
(* side effects of Cmts.fmt c.cmts before Sugar.fun_ is
1815+
important *)
18131816
let cmts_before = Cmts.fmt_before c pexp_loc in
18141817
let xargs, xbody = Sugar.fun_ c.cmts (sub_exp ~ctx eN1) in
18151818
let fmt_cstr, xbody = type_constr_and_body c xbody in

Diff for: ‎test/passing/tests/align_cases-break_all.ml.ref

+2-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ let fooooooooooo =
5757
foo
5858
| 3453535353533 ->
5959
foooooooooooooooooo
60-
(* foooooooooooooooooooooo foooooooooooooooo foooooooooooooo fooooooooo*)
60+
(* foooooooooooooooooooooo foooooooooooooooo foooooooooooooo
61+
fooooooooo*)
6162
| _ -> fooooooooooooooooooo
6263

6364
let _ =

Diff for: ‎test/passing/tests/align_cases.ml

+2-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ let fooooooooooo =
5757
foo
5858
| 3453535353533 ->
5959
foooooooooooooooooo
60-
(* foooooooooooooooooooooo foooooooooooooooo foooooooooooooo fooooooooo*)
60+
(* foooooooooooooooooooooo foooooooooooooooo foooooooooooooo
61+
fooooooooo*)
6162
| _ -> fooooooooooooooooooo
6263

6364
let _ =

Diff for: ‎test/passing/tests/break_cases-align.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,8 @@ let foooooooooooooo = function
265265
| Foooooooooo
266266
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
267267
| Foooo
268-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
269-
->
268+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
269+
fooo *) ->
270270
Foooooooooo.Foooooo
271271
| Foooo {foooo_fooo= {foooooooooo}} ->
272272
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -275,8 +275,8 @@ let get_nullability = function
275275
| ArrayAccess
276276
| OptimisticFallback (* non-null is the most optimistic type *)
277277
| Undef
278-
(* This is a very special case, assigning non-null is a technical trick *)
279-
->
278+
(* This is a very special case, assigning non-null is a technical
279+
trick *) ->
280280
Nullability.Nonnull
281281

282282
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-all.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,8 @@ let foooooooooooooo = function
265265
| Foooooooooo
266266
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
267267
| Foooo
268-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
269-
->
268+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
269+
fooo *) ->
270270
Foooooooooo.Foooooo
271271
| Foooo {foooo_fooo= {foooooooooo}} ->
272272
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -275,8 +275,8 @@ let get_nullability = function
275275
| ArrayAccess
276276
| OptimisticFallback (* non-null is the most optimistic type *)
277277
| Undef
278-
(* This is a very special case, assigning non-null is a technical trick *)
279-
->
278+
(* This is a very special case, assigning non-null is a technical
279+
trick *) ->
280280
Nullability.Nonnull
281281

282282
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-closing_on_separate_line.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,8 @@ let foooooooooooooo = function
280280
| Foooooooooo
281281
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
282282
| Foooo
283-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
284-
->
283+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
284+
fooo *) ->
285285
Foooooooooo.Foooooo
286286
| Foooo {foooo_fooo= {foooooooooo}} ->
287287
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -290,8 +290,8 @@ let get_nullability = function
290290
| ArrayAccess
291291
| OptimisticFallback (* non-null is the most optimistic type *)
292292
| Undef
293-
(* This is a very special case, assigning non-null is a technical trick *)
294-
->
293+
(* This is a very special case, assigning non-null is a technical
294+
trick *) ->
295295
Nullability.Nonnull
296296

297297
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-closing_on_separate_line_leading_nested_match_parens.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,8 @@ let foooooooooooooo = function
280280
| Foooooooooo
281281
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
282282
| Foooo
283-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
284-
->
283+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
284+
fooo *) ->
285285
Foooooooooo.Foooooo
286286
| Foooo {foooo_fooo= {foooooooooo}} ->
287287
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -290,8 +290,8 @@ let get_nullability = function
290290
| ArrayAccess
291291
| OptimisticFallback (* non-null is the most optimistic type *)
292292
| Undef
293-
(* This is a very special case, assigning non-null is a technical trick *)
294-
->
293+
(* This is a very special case, assigning non-null is a technical
294+
trick *) ->
295295
Nullability.Nonnull
296296

297297
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-cosl_lnmp_cmei.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,8 @@ let foooooooooooooo = function
280280
| Foooooooooo
281281
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
282282
| Foooo
283-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
284-
->
283+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
284+
fooo *) ->
285285
Foooooooooo.Foooooo
286286
| Foooo {foooo_fooo= {foooooooooo}} ->
287287
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -290,8 +290,8 @@ let get_nullability = function
290290
| ArrayAccess
291291
| OptimisticFallback (* non-null is the most optimistic type *)
292292
| Undef
293-
(* This is a very special case, assigning non-null is a technical trick *)
294-
->
293+
(* This is a very special case, assigning non-null is a technical
294+
trick *) ->
295295
Nullability.Nonnull
296296

297297
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-fit_or_vertical.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -226,17 +226,17 @@ let foooooooooooooo = function
226226
| Foooooooooo
227227
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
228228
| Foooo
229-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
230-
-> Foooooooooo.Foooooo
229+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
230+
fooo *) -> Foooooooooo.Foooooo
231231
| Foooo {foooo_fooo= {foooooooooo}} ->
232232
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
233233

234234
let get_nullability = function
235235
| ArrayAccess
236236
| OptimisticFallback (* non-null is the most optimistic type *)
237237
| Undef
238-
(* This is a very special case, assigning non-null is a technical trick *)
239-
-> Nullability.Nonnull
238+
(* This is a very special case, assigning non-null is a technical
239+
trick *) -> Nullability.Nonnull
240240

241241
[@@@ocamlformat "exp-grouping=preserve"]
242242

Diff for: ‎test/passing/tests/break_cases-nested.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,8 @@ let foooooooooooooo = function
231231
| Foooooooooo
232232
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
233233
| Foooo
234-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
235-
->
234+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
235+
fooo *) ->
236236
Foooooooooo.Foooooo
237237
| Foooo {foooo_fooo= {foooooooooo}} ->
238238
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -241,8 +241,8 @@ let get_nullability = function
241241
| ArrayAccess
242242
| OptimisticFallback (* non-null is the most optimistic type *)
243243
| Undef
244-
(* This is a very special case, assigning non-null is a technical trick *)
245-
->
244+
(* This is a very special case, assigning non-null is a technical
245+
trick *) ->
246246
Nullability.Nonnull
247247

248248
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-normal_indent.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,8 @@ let foooooooooooooo = function
265265
| Foooooooooo
266266
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
267267
| Foooo
268-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
269-
->
268+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
269+
fooo *) ->
270270
Foooooooooo.Foooooo
271271
| Foooo {foooo_fooo= {foooooooooo}} ->
272272
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -275,8 +275,8 @@ let get_nullability = function
275275
| ArrayAccess
276276
| OptimisticFallback (* non-null is the most optimistic type *)
277277
| Undef
278-
(* This is a very special case, assigning non-null is a technical trick *)
279-
->
278+
(* This is a very special case, assigning non-null is a technical
279+
trick *) ->
280280
Nullability.Nonnull
281281

282282
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases-toplevel.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -231,8 +231,8 @@ let foooooooooooooo = function
231231
| Foooooooooo
232232
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
233233
| Foooo
234-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
235-
->
234+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
235+
fooo *) ->
236236
Foooooooooo.Foooooo
237237
| Foooo {foooo_fooo= {foooooooooo}} ->
238238
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -241,8 +241,8 @@ let get_nullability = function
241241
| ArrayAccess
242242
| OptimisticFallback (* non-null is the most optimistic type *)
243243
| Undef
244-
(* This is a very special case, assigning non-null is a technical trick *)
245-
->
244+
(* This is a very special case, assigning non-null is a technical
245+
trick *) ->
246246
Nullability.Nonnull
247247

248248
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_cases.ml.ref

+4-4
Original file line numberDiff line numberDiff line change
@@ -203,8 +203,8 @@ let foooooooooooooo = function
203203
| Foooooooooo
204204
| FooooFoooooFoooooo (* fooooooooooooooooooooooooooooooooooo *)
205205
| Foooo
206-
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo fooo *)
207-
->
206+
(* Fooo foooo fooooo foooooooo fooooooooo foooooooooooo fooooooooo
207+
fooo *) ->
208208
Foooooooooo.Foooooo
209209
| Foooo {foooo_fooo= {foooooooooo}} ->
210210
Foooo_Foooo_fooooooo.get_foooooooooo fooooo_fooo
@@ -213,8 +213,8 @@ let get_nullability = function
213213
| ArrayAccess
214214
| OptimisticFallback (* non-null is the most optimistic type *)
215215
| Undef
216-
(* This is a very special case, assigning non-null is a technical trick *)
217-
->
216+
(* This is a very special case, assigning non-null is a technical
217+
trick *) ->
218218
Nullability.Nonnull
219219

220220
[@@@ocamlformat "exp-grouping=preserve"]

Diff for: ‎test/passing/tests/break_separators-after.ml.ref

+2-1
Original file line numberDiff line numberDiff line change
@@ -521,7 +521,8 @@ let foooooooooooo =
521521

522522
let foooooooooooo =
523523
{ foooooooooooooo with
524-
(* foooooooooooooooo fooooooooooooooooooooooooo foooooooooooooooooooooo *)
524+
(* foooooooooooooooo fooooooooooooooooooooooooo
525+
foooooooooooooooooooooo *)
525526
fooooooooooooooooooooooooooooo= fooooooooooooo;
526527
fooooooooooooo= foooooooooooooo }
527528

Diff for: ‎test/passing/tests/break_separators-after_docked.ml.ref

+2-1
Original file line numberDiff line numberDiff line change
@@ -560,7 +560,8 @@ let foooooooooooo =
560560
let foooooooooooo =
561561
{
562562
foooooooooooooo with
563-
(* foooooooooooooooo fooooooooooooooooooooooooo foooooooooooooooooooooo *)
563+
(* foooooooooooooooo fooooooooooooooooooooooooo
564+
foooooooooooooooooooooo *)
564565
fooooooooooooooooooooooooooooo= fooooooooooooo;
565566
fooooooooooooo= foooooooooooooo;
566567
}

0 commit comments

Comments
 (0)