Always treat multiline {|delimited strings|} as though they are long
#2480
Conversation
| @@ -1,4 +1,5 @@ | |||
| let _ = {| | |||
| let _ = | |||
| {| | |||
There was a problem hiding this comment.
The previous output was expected. We call this "docking" in other contextes.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -185,11 +185,13 @@ let this_function_has_a_long_name plus very many arguments = | |||
|
|
|||
| [%%expect {||}] ;; | |||
|
|
|||
| [%expect {| | |||
| [%expect | |||
| {| | |||
There was a problem hiding this comment.
Same as with let, the previous output is expected.
test/passing/gen/gen.ml
Outdated
| spf | ||
| {|(with-accepted-exit-codes 1 | ||
| (run %s))|} | ||
| cmd_string |
There was a problem hiding this comment.
This output can be achieved by tweaking fmt_args_grouped instead of adding breaks as experimented in #2448
lib/Fmt_ast.ml
Outdated
| (* 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 "") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for pointing this out, I'll take a look at the PR you linked above and try to come up with a fix.
There was a problem hiding this comment.
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.
after this commit, only the js_source.ml.* tests change as compared to the trunk
| @@ -3154,7 +3154,8 @@ module FM_valid = F (struct | |||
| type t = int | |||
| end) | |||
|
|
|||
| [%%expect {| | |||
| [%%expect | |||
| {| | |||
There was a problem hiding this comment.
The docking behavior could be specially supported again if needed.
Currently, ocamlformat determines whether a delimited string is "long" enough to split a box across multiple lines simply by its length. However, a multiline string should always be treated as long enough to split a box, even if it doesn't have so many characters. This feature implements the desired behavior.
We implement the fix by exposing
str_asinFmt.mliso that we can use it to append short strings that get treated as very long after multiline strings.