Skip to content

Always treat multiline {|delimited strings|} as though they are long #2480

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Always treat multiline {|delimited strings|} as though they are long #2480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0162779
always treat multiline {||} strings as though they are long
dvulakh Nov 15, 2023
df34d81
only change treatment of multiline {||} in janestreet profile
dvulakh Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/Fmt.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ val char : char -> t
val str : string -> t
(** Format a string. *)

val str_as : int -> string -> t
(** [str_as a len] formats a string as if it were of length [len]. *)

(** Primitive containers ------------------------------------------------*)

val opt : 'a option -> ('a -> t) -> t
Expand Down
31 changes: 17 additions & 14 deletions lib/Fmt_ast.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,11 @@ let fmt_constant c ?epi {pconst_desc; pconst_loc= loc} =
str lit $ opt suf char
| Pconst_char (_, s) -> wrap "'" "'" @@ str s
| Pconst_string (s, loc', Some delim) ->
Cmts.fmt c loc'
@@ wrap_k (str ("{" ^ delim ^ "|")) (str ("|" ^ delim ^ "}")) (str s)
Cmts.fmt c loc' @@ str (Format_.sprintf "{%s|%s|%s}" delim s delim)
(* If a multiline string has newlines in it, it should get treated as a
"long" box element. To do so, we append a length-1000 empty
string. *)
$ fmt_if_k (String.mem s '\n') (str_as 1000 "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's generally not a good idea to put a break at the end of a box as it interferes with the formatting of the containing expression:

let _ =
  {|
    foo
    bar
  |}
  ;
  ""

Instead, we write rules that can be followed when formatting the containing expression. For example, is_simple, break_between, and more defined in Ast. This is more or less done in fmt_args_grouped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I'll take a look at the PR you linked above and try to come up with a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that, as far as I can tell, semicolons after multiline strings still format fine in the janestreet profile.

I have pushed a commit that restricts the new behavior to the janestreet profile, though.

| Pconst_string (_, loc', None) -> (
let delim = ["@,"; "@;"] in
let contains_pp_commands s =
Expand Down Expand Up @@ -513,18 +516,18 @@ let sequence_blank_line c (l1 : Location.t) (l2 : Location.t) =
loop l1.loc_end (Cmts.remaining_before c.cmts l2)
| `Compact -> false

let fmt_quoted_string key ext s = function
| None ->
wrap_k (str (Format_.sprintf "{%s%s|" key ext)) (str "|}") (str s)
| Some delim ->
let ext_and_delim =
if String.is_empty delim then ext
else Format_.sprintf "%s %s" ext delim
in
wrap_k
(str (Format_.sprintf "{%s%s|" key ext_and_delim))
(str (Format_.sprintf "|%s}" delim))
(str s)
let fmt_quoted_string key ext s maybe_delim =
let s_fmt =
match maybe_delim with
| None -> str (Format_.sprintf "{%s%s|%s|}" key ext s)
| Some delim ->
let ext_and_delim =
if String.is_empty delim then ext
else Format_.sprintf "%s %s" ext delim
in
str (Format_.sprintf "{%s%s|%s|%s}" key ext_and_delim s delim)
in
s_fmt $ fmt_if_k (String.mem s '\n') (str_as 1000 "")

let fmt_type_var s =
str "'"
Expand Down
6 changes: 4 additions & 2 deletions test/failing/gen/gen.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ let register_file tests fname =

let cmd args =
let cmd_string = String.concat " " args in
Printf.sprintf {|(with-accepted-exit-codes 1
(run %s))|} cmd_string
Printf.sprintf
{|(with-accepted-exit-codes 1
(run %s))|}
cmd_string

let emit_test test_name setup =
let opts =
Expand Down
6 changes: 4 additions & 2 deletions test/passing/gen/gen.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ let register_file tests fname =
let cmd should_fail args =
let cmd_string = String.concat " " args in
if should_fail then
spf {|(with-accepted-exit-codes 1
(run %s))|} cmd_string
spf
{|(with-accepted-exit-codes 1
(run %s))|}
cmd_string
Copy link
Collaborator

Choose a reason for hiding this comment

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

This output can be achieved by tweaking fmt_args_grouped instead of adding breaks as experimented in #2448

else spf {|(run %s)|} cmd_string

let emit_test test_name setup =
Expand Down
3 changes: 2 additions & 1 deletion test/passing/tests/crlf_to_crlf.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
let _ = {|
let _ =
{|
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous output was expected. We call this "docking" in other contextes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll restrict the change to the janestreet profile, then. It is surprising to me, though, that the property used to determine whether to "dock" is the number of characters in the string and not something that more closely relates to how it "looks", like whether the first line is empty, the number of lines, or the length of the longest line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's also surprising to me. I tried various rules in #2448 without success, as the changes to existing code is always large and the rule is always arbitrary. Thanks for the ideas, I'll try to come back to this change later.

foo

bar
Expand Down
3 changes: 2 additions & 1 deletion test/passing/tests/crlf_to_lf.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
let _ = {|
let _ =
{|
foo

bar
Expand Down
6 changes: 4 additions & 2 deletions test/passing/tests/extensions-indent.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,13 @@ let this_function_has_a_long_name plus very many arguments =

[%%expect {||}] ;;

[%expect {|
[%expect
{|
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as with let, the previous output is expected.

___________________________________________________________
|}]

[%%expect {|
[%%expect
{|
___________________________________________________________
|}]

Expand Down
6 changes: 4 additions & 2 deletions test/passing/tests/extensions.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,13 @@ let this_function_has_a_long_name plus very many arguments =

[%%expect {||}] ;;

[%expect {|
[%expect
{|
___________________________________________________________
|}]

[%%expect {|
[%%expect
{|
___________________________________________________________
|}]

Expand Down
8 changes: 4 additions & 4 deletions test/passing/tests/js_source.ml.err
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Warning: tests/js_source.ml:155 exceeds the margin
Warning: tests/js_source.ml:9531 exceeds the margin
Warning: tests/js_source.ml:9634 exceeds the margin
Warning: tests/js_source.ml:9693 exceeds the margin
Warning: tests/js_source.ml:9775 exceeds the margin
Warning: tests/js_source.ml:9537 exceeds the margin
Warning: tests/js_source.ml:9640 exceeds the margin
Warning: tests/js_source.ml:9699 exceeds the margin
Warning: tests/js_source.ml:9781 exceeds the margin
24 changes: 16 additions & 8 deletions test/passing/tests/js_source.ml.ocp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3154,7 +3154,8 @@ module FM_valid = F (struct
type t = int
end)

[%%expect {|
[%%expect
{|
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docking behavior could be specially supported again if needed.

module M_valid : S
module FM_valid : S
|}]
Expand All @@ -3170,7 +3171,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Foo : sig type t val x : t ref end
|}]

Expand All @@ -3184,7 +3186,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Bar : sig type t [@@immediate] val x : t ref end
|}]

Expand All @@ -3194,7 +3197,8 @@ let test f =
Sys.time () -. start
;;

[%%expect {|
[%%expect
{|
val test : (unit -> 'a) -> float = <fun>
|}]

Expand All @@ -3204,7 +3208,8 @@ let test_foo () =
done
;;

[%%expect {|
[%%expect
{|
val test_foo : unit -> unit = <fun>
|}]

Expand All @@ -3214,7 +3219,8 @@ let test_bar () =
done
;;

[%%expect {|
[%%expect
{|
val test_bar : unit -> unit = <fun>
|}]

Expand Down Expand Up @@ -10330,8 +10336,10 @@ zzzzzzzzzzzzzzzzzzzzzzzzzzzz
*)
(*$*)

(*$ {|
f|} *)
(*$
{|
f|}
*)

let () =
match () with
Expand Down
24 changes: 16 additions & 8 deletions test/passing/tests/js_source.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3154,7 +3154,8 @@ module FM_valid = F (struct
type t = int
end)

[%%expect {|
[%%expect
{|
module M_valid : S
module FM_valid : S
|}]
Expand All @@ -3170,7 +3171,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Foo : sig type t val x : t ref end
|}]

Expand All @@ -3184,7 +3186,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Bar : sig type t [@@immediate] val x : t ref end
|}]

Expand All @@ -3194,7 +3197,8 @@ let test f =
Sys.time () -. start
;;

[%%expect {|
[%%expect
{|
val test : (unit -> 'a) -> float = <fun>
|}]

Expand All @@ -3204,7 +3208,8 @@ let test_foo () =
done
;;

[%%expect {|
[%%expect
{|
val test_foo : unit -> unit = <fun>
|}]

Expand All @@ -3214,7 +3219,8 @@ let test_bar () =
done
;;

[%%expect {|
[%%expect
{|
val test_bar : unit -> unit = <fun>
|}]

Expand Down Expand Up @@ -10330,8 +10336,10 @@ zzzzzzzzzzzzzzzzzzzzzzzzzzzz
*)
(*$*)

(*$ {|
f|} *)
(*$
{|
f|}
*)

let () =
match () with
Expand Down
18 changes: 12 additions & 6 deletions test/passing/tests/source.ml.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3006,7 +3006,8 @@ module FM_valid = F (struct
type t = int
end)

[%%expect {|
[%%expect
{|
module M_valid : S
module FM_valid : S
|}]
Expand All @@ -3022,7 +3023,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Foo : sig type t val x : t ref end
|}]

Expand All @@ -3036,7 +3038,8 @@ end = struct
let x = ref 0
end

[%%expect {|
[%%expect
{|
module Bar : sig type t [@@immediate] val x : t ref end
|}]

Expand All @@ -3045,7 +3048,8 @@ let test f =
f () ;
Sys.time () -. start

[%%expect {|
[%%expect
{|
val test : (unit -> 'a) -> float = <fun>
|}]

Expand All @@ -3054,7 +3058,8 @@ let test_foo () =
Foo.x := !Foo.x
done

[%%expect {|
[%%expect
{|
val test_foo : unit -> unit = <fun>
|}]

Expand All @@ -3063,7 +3068,8 @@ let test_bar () =
Bar.x := !Bar.x
done

[%%expect {|
[%%expect
{|
val test_bar : unit -> unit = <fun>
|}]

Expand Down