From ffc72e903d66c09ab0c7e0eccfbf0ada42d09586 Mon Sep 17 00:00:00 2001 From: Jules Aguillon Date: Tue, 3 Oct 2023 15:22:36 +0200 Subject: [PATCH 1/4] Add failing test --- .../tests/break_string_literals-never.ml.err | 2 ++ .../tests/break_string_literals-never.ml.ref | 10 ++++++++++ test/passing/tests/break_string_literals.ml | 14 ++++++++++++++ test/passing/tests/break_string_literals.ml.ref | 12 ++++++++++++ 4 files changed, 38 insertions(+) diff --git a/test/passing/tests/break_string_literals-never.ml.err b/test/passing/tests/break_string_literals-never.ml.err index 1a73d22483..a181d8b583 100644 --- a/test/passing/tests/break_string_literals-never.ml.err +++ b/test/passing/tests/break_string_literals-never.ml.err @@ -3,3 +3,5 @@ Warning: tests/break_string_literals.ml:7 exceeds the margin Warning: tests/break_string_literals.ml:11 exceeds the margin Warning: tests/break_string_literals.ml:48 exceeds the margin Warning: tests/break_string_literals.ml:51 exceeds the margin +Warning: tests/break_string_literals.ml:63 exceeds the margin +Warning: tests/break_string_literals.ml:68 exceeds the margin diff --git a/test/passing/tests/break_string_literals-never.ml.ref b/test/passing/tests/break_string_literals-never.ml.ref index 34817d10a0..a4ac9d25bc 100644 --- a/test/passing/tests/break_string_literals-never.ml.ref +++ b/test/passing/tests/break_string_literals-never.ml.ref @@ -58,3 +58,13 @@ let _ = "abc@,def\n\n ghi" let _ = "abc@,def\n\n" let _ = "abc@,def@\n\n" + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s\nPermitted values: if-exists always never\nDefault: %s" + var v (to_string default) + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s Permitted values: if-exists always never Default: %s" + var v (to_string default) diff --git a/test/passing/tests/break_string_literals.ml b/test/passing/tests/break_string_literals.ml index 72be2905af..640a4a6edc 100644 --- a/test/passing/tests/break_string_literals.ml +++ b/test/passing/tests/break_string_literals.ml @@ -69,3 +69,17 @@ let _ = "abc@,def\n\nghi" let _ = "abc@,def\n\n ghi" let _ = "abc@,def\n\n" let _ = "abc@,def@\n\n" + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s\n\ + Permitted values: if-exists always never\n\ + Default: %s" + var v (to_string default) + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s \ + Permitted values: if-exists always never \ + Default: %s" + var v (to_string default) diff --git a/test/passing/tests/break_string_literals.ml.ref b/test/passing/tests/break_string_literals.ml.ref index 9afe262471..56e5e9dd20 100644 --- a/test/passing/tests/break_string_literals.ml.ref +++ b/test/passing/tests/break_string_literals.ml.ref @@ -92,3 +92,15 @@ let _ = "abc@,def\n\n ghi" let _ = "abc@,def\n\n" let _ = "abc@,def@\n\n" + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s\n\ + Permitted values: if-exists always never\n\ + Default: %s" var v (to_string default) + +let _ = + Pp.textf + "Failed to parse environment variable: %s=%s Permitted values: \ + if-exists always never Default: %s" + var v (to_string default) From 3cd91fae1c6bc7a89cf76c3cf14ce9efe26f3989 Mon Sep 17 00:00:00 2001 From: Jules Aguillon Date: Tue, 3 Oct 2023 15:35:25 +0200 Subject: [PATCH 2/4] Consistent break after string constant argument A break is added after wrapping string constants in argument lists. This break was missing for string constants containing explicit line breaks or format hints. --- lib/Fmt_ast.ml | 12 ++++++------ test/passing/tests/break_string_literals.ml.ref | 3 ++- test/passing/tests/js_args.ml.ref | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/Fmt_ast.ml b/lib/Fmt_ast.ml index 26341f8559..b878ab277d 100644 --- a/lib/Fmt_ast.ml +++ b/lib/Fmt_ast.ml @@ -1467,15 +1467,15 @@ and fmt_args_grouped ?epi:(global_epi = noop) c ctx args = | Pexp_fun _ | Pexp_function _ -> Some false | _ -> None in - let epi = - match (lbl, last) with - | _, true -> None - | Nolabel, _ -> Some (fits_breaks "" ~hint:(1000, -1) "") - | _ -> Some (fits_breaks "" ~hint:(1000, -3) "") + let break_after = + match (ast.pexp_desc, c.conf.fmt_opts.break_string_literals.v) with + | Pexp_constant _, `Auto when not last -> + fits_breaks "" ~hint:(1000, -2) "" + | _ -> noop in hovbox (Params.Indent.fun_args_group c.conf ~lbl ast) - (fmt_label_arg c ?box ?epi (lbl, xarg)) + (fmt_label_arg c ?box (lbl, xarg) $ break_after) $ fmt_if_k (not last) (break_unless_newline 1 0) in let fmt_args ~first ~last args = diff --git a/test/passing/tests/break_string_literals.ml.ref b/test/passing/tests/break_string_literals.ml.ref index 56e5e9dd20..d09646eff8 100644 --- a/test/passing/tests/break_string_literals.ml.ref +++ b/test/passing/tests/break_string_literals.ml.ref @@ -97,7 +97,8 @@ let _ = Pp.textf "Failed to parse environment variable: %s=%s\n\ Permitted values: if-exists always never\n\ - Default: %s" var v (to_string default) + Default: %s" + var v (to_string default) let _ = Pp.textf diff --git a/test/passing/tests/js_args.ml.ref b/test/passing/tests/js_args.ml.ref index 8addea5617..cfdd91ada4 100644 --- a/test/passing/tests/js_args.ml.ref +++ b/test/passing/tests/js_args.ml.ref @@ -33,7 +33,8 @@ let () = messages := Message_store.create (Session_id.of_string "") (* Tuareg indents these lines too far to the left. *) - "herd-retransmitter" Message_store.Message_size.Byte + "herd-retransmitter" + Message_store.Message_size.Byte let () = raise From 3c9cd77375a98692a63679fe775f9dd56b9b55d6 Mon Sep 17 00:00:00 2001 From: Jules Aguillon Date: Tue, 3 Oct 2023 15:41:21 +0200 Subject: [PATCH 3/4] Update CHANGES --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index e5a4246e2f..83cd9ab638 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,6 +15,7 @@ profile. This started with version 0.26.0. - Remove trailing space inside a wrapping empty signature (#2443, @Julow) - Fix extension-point spacing in structures (#2450, @Julow) +- \* Consistent break after string constant argument (#2453, @Julow) ## 0.26.1 (2023-09-15) From 4e96326b30333ca599f344a4198def5ee023aa97 Mon Sep 17 00:00:00 2001 From: Jules Aguillon Date: Thu, 5 Oct 2023 14:17:33 +0200 Subject: [PATCH 4/4] Remove now unused `fmt_label_arg ?epi` --- lib/Fmt_ast.ml | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/Fmt_ast.ml b/lib/Fmt_ast.ml index b878ab277d..54ec5e33dc 100644 --- a/lib/Fmt_ast.ml +++ b/lib/Fmt_ast.ml @@ -1417,7 +1417,7 @@ and fmt_fun ?force_closing_paren $ body $ closing $ Cmts.fmt_after c ast.pexp_loc ) -and fmt_label_arg ?(box = true) ?epi ?eol c (lbl, ({ast= arg; _} as xarg)) = +and fmt_label_arg ?(box = true) ?eol c (lbl, ({ast= arg; _} as xarg)) = match (lbl, arg.pexp_desc) with | (Labelled l | Optional l), Pexp_ident {txt= Lident i; loc} when String.equal l.txt i && List.is_empty arg.pexp_attributes -> @@ -1435,15 +1435,13 @@ and fmt_label_arg ?(box = true) ?epi ?eol c (lbl, ({ast= arg; _} as xarg)) = | Optional _ -> str "?" | Nolabel -> noop in - lbl $ fmt_expression c ~box ?epi xarg + lbl $ fmt_expression c ~box xarg | (Labelled _ | Optional _), _ when Cmts.has_after c.cmts xarg.ast.pexp_loc -> let cmts_after = Cmts.fmt_after c xarg.ast.pexp_loc in hvbox_if box 2 ( hvbox_if box 0 - (fmt_expression c - ~pro:(fmt_label lbl ":@;<0 2>") - ~box ?epi xarg ) + (fmt_expression c ~pro:(fmt_label lbl ":@;<0 2>") ~box xarg) $ cmts_after ) | (Labelled _ | Optional _), (Pexp_fun _ | Pexp_newtype _) -> fmt_fun ~box ~label:lbl ~parens:true c xarg @@ -1451,7 +1449,7 @@ and fmt_label_arg ?(box = true) ?epi ?eol c (lbl, ({ast= arg; _} as xarg)) = let label_sep : s = if box || c.conf.fmt_opts.wrap_fun_args.v then ":@," else ":" in - fmt_label lbl label_sep $ fmt_expression c ~box ?epi xarg + fmt_label lbl label_sep $ fmt_expression c ~box xarg and expression_width c xe = String.length