Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases#1382
Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases#1382gpetiot merged 6 commits intoocaml-ppx:masterfrom gpetiot:remove-trailing-space-attr
Conversation
|
Just for the record: I do not consider trailing spaces to be a "bug". It's nice not to generate them, but it is really not something that I would want to complicate the code too much over. But that is just my opinion. I do not recall any particular need to place attributes on the same line instead of letting a newline appear before them. But IIRC @hhugo added most of the handling of attributes since early on attributes were just dropped. Any recollection Hugo? |
|
I do not recall any reason |
|
I extended the change to more constructs to have an idea of what it will look like. Most of the time having the attributes at the beginning of the line make it easier to read and see to what it is attached to. |
| @ b________________________________________________________________ ) | ||
| [@a] |
There was a problem hiding this comment.
Looks like this attribute should fit. If not, this test isn't checking the property it was intended for anymore and should be modified.
There was a problem hiding this comment.
I think this test still wants either modified or removed. I introduced it as part of #1306 to guard against a bug where the attribute was split across multiple lines on wrap.
There was a problem hiding this comment.
Ok. I don't have a strong position either way. @gpetiot feel free to include a change to this test in this PR as you see fit.
jberdine
left a comment
There was a problem hiding this comment.
I don't know if the interaction with ocp-indent is important, but the rest of this PR is good.
|
This change is now only applied if |
lib/Fmt_ast.ml
Outdated
|
|
||
| (* Breaking before an attribute can confuse ocp-indent that will produce a | ||
| suboptimal indentation. *) | ||
| let pre_attributes c = fmt_or c.conf.ocp_indent_compat " " "@ " |
There was a problem hiding this comment.
I'm sorry for not thinking of this suggestion before, but I am inclined to try a different implementation, where instead of adding the pre_attributes function, fmt_attributes is changed to test if pre is "@ " and if so change it to " ". That will more tightly localize the mess made by ocp-indent compatibility.
There was a problem hiding this comment.
Good point, it looks better indeed. No regression with test_branch.
CHANGES:
#### Changes
+ Do not break inline elements such as `{i blah}` in docstrings (ocaml-ppx/ocamlformat#1346, @jberdine)
+ Distinguish hash-getter from hash-comparison infix operators. Operators of the form `#**#` or `#**.` where `**` can be 0 or more operator chars are considered getter operators and are not surrounded by spaces, as opposed to regular infix operators (ocaml-ppx/ocamlformat#1376, @gpetiot)
+ Type constraint on return type of functions is now always printed before the function body (ocaml-ppx/ocamlformat#1381, ocaml-ppx/ocamlformat#1397, @gpetiot)
#### Bug fixes
+ Restore previous functionality for pre-post extension points (ocaml-ppx/ocamlformat#1342, @jberdine)
+ Fix extra break before `function` body of a `fun` (ocaml-ppx/ocamlformat#1343, @jberdine)
Indent further args of anonymous functions (ocaml-ppx/ocamlformat#1440, @gpetiot)
+ Do not clear the emacs `*compilation*` buffer on successful reformat (ocaml-ppx/ocamlformat#1350, @jberdine)
+ Fix disabling with attributes on OCaml < 4.08 (ocaml-ppx/ocamlformat#1322, @emillon)
+ Preserve unwrapped comments by not adding artificial breaks when `wrap-comments=false` and `ocp-indent-compat=true` are set to avoid interfering with ocp-indent indentation. (ocaml-ppx/ocamlformat#1352, @gpetiot)
+ Break long literal strings at the margin (ocaml-ppx/ocamlformat#1367, @gpetiot)
+ Break after a multiline argument in an argument list (ocaml-ppx/ocamlformat#1360, @gpetiot)
+ Remove unnecessary parens around object (ocaml-ppx/ocamlformat#1379, @gpetiot)
+ Fix placement of comments on constants (ocaml-ppx/ocamlformat#1383, @gpetiot)
+ Do not escape arguments of some Odoc tags (ocaml-ppx/ocamlformat#1391, 1408, @gpetiot, @Julow)
The characters `[]{}` must not be escaped in the arguments of `@raise`, `@author`, `@version` and others.
+ Fix missing open line between multi-line let-binding with poly-typexpr (ocaml-ppx/ocamlformat#1372, @jberdine)
+ Remove trailing space after expression when followed by an attribute and break before attributes attached to multi-line phrases (ocaml-ppx/ocamlformat#1382, @gpetiot)
+ Do not add a space to minimal comments `(* *)`, `(** *)` and `(*$ *)` (ocaml-ppx/ocamlformat#1407, @gpetiot)
+ Fix attributes position in labelled arguments type (ocaml-ppx/ocamlformat#1434, @gpetiot)
+ Add missing parens around type annotation in anonymous function (ocaml-ppx/ocamlformat#1433, @gpetiot)
+ Fix alignment of 'then' keyword in parenthesised expression (ocaml-ppx/ocamlformat#1421, @gpetiot)
#### New features
+ Support quoted extensions (added in ocaml 4.11) (ocaml-ppx/ocamlformat#1405, @gpetiot)
+ Recognise eliom file extensions (ocaml-ppx/ocamlformat#1430, @jrochel)
fix #1356
I had to change the kind of boxes in a lot of places to maintain the current behavior of wrapping the attribute at the end of the previous line when possible.
But
fmt_attributes c ~pre:(str " ")is still used in a lot of places so there must be more issues like this, @jberdine was thispreintended? Should we preserve the current behavior or is it okay to put the attribute in the next line instead of wrapping at the end of the previous line?