Retain attribute on pexp_sequence#1291
Merged
Julow merged 5 commits intoocaml-ppx:masterfrom Mar 20, 2020
Merged
Conversation
|
Hi @craigfe! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Julow
reviewed
Mar 18, 2020
Collaborator
Julow
left a comment
There was a problem hiding this comment.
You need to promote the tests again, we have a test that runs from the result of an other.
About the added parentheses, you can add a special case to the beautiful parenze_exp function in Ast. Something like Exp {pexp_desc= Pexp_sequence _}, {pexp_attributes=_::_}
Collaborator
|
no sarcasm please :) |
ce70430 to
b4bed9a
Compare
Contributor
Author
|
This one is more subtle than the others. In doing this, I removed some unnecessary parentheses (b4bed9a) in a way that is breaking (see the change in This PR also made me realise that the parentheses that we're adding due to attributes are not like the ones added by let _ =
(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ;
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb) [@a]
let _ =
f
( aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ;
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb )The second parentheses are added by I chose not to open this can of worms here, since it's an existing inconsistency. |
Julow
approved these changes
Mar 18, 2020
Collaborator
Julow
left a comment
There was a problem hiding this comment.
I chose not to open this can of worms here, since it's an existing inconsistency.
We can do that later ! This PR is already only fixes.
Looks good to me :) I couldn't find edge cases to the new rule in parenze_exp.
| [@ocaml.warning "-3"]); | ||
| fprintf ppf "open %s\n\n" | ||
| (String.capitalize (Filename.basename Grammar.basename)) | ||
| [@ocaml.warning "-3"]; |
Collaborator
There was a problem hiding this comment.
test_branch found 2 occurrences of this on Dune. Still looks like a fix.
eb5ac6b to
67768f6
Compare
1fc2505 to
b8cb650
Compare
b8cb650 to
b51e118
Compare
Collaborator
|
No regressions with |
Julow
added a commit
to Julow/opam-repository
that referenced
this pull request
Apr 2, 2020
CHANGES: #### New features + Add an option `--format-invalid-files` to print unparsable parts of the input as verbatim text. This feature is still experimental. (ocaml-ppx/ocamlformat#1026) (Guillaume Petiot) + Support multi-indices extended indexing operators (ocaml-ppx/ocamlformat#1279, ocaml-ppx/ocamlformat#1277) (Jules Aguillon, Guillaume Petiot) This feature has been added in OCaml 4.10.0 + Handle OCaml 4.10.0 AST (ocaml-ppx/ocamlformat#1276) (Guillaume Petiot) + Preserve functor syntax for consistency (ocaml-ppx/ocamlformat#1312) (Guillaume Petiot) Previously both functor syntax: `module M = functor (K : S) -> struct end` and `module M (K : S) = struct end` would be formatted as the latter, the original syntax is now preserved. #### Changes + Add the option `doc-comments-val=before|after` (ocaml-ppx/ocamlformat#1012) (Jules Aguillon) This option set the placement of documentation comment on `val` and `external` only. It is set to `after` by default. + The default for `doc-comments` is changed from `after` to `before` (ocaml-ppx/ocamlformat#1012, ocaml-ppx/ocamlformat#1325) (Jules Aguillon) This affects both `conventional` (default) and `ocamlformat` profiles. + Some options are now deprecated: * `doc-comments` (ocaml-ppx/ocamlformat#1293, ocaml-ppx/ocamlformat#1012) This option depends on a flawed heuristic. It is replaced by `doc-comments-val` for `val` and `external` declarations. There is no equivalent to this option in the general case. * `escape-chars`, `escape-strings` and `extension-sugar` (ocaml-ppx/ocamlformat#1293) These options are rarely used and their default behavior is considered to be the right behavior. + Add space between `row_field` attributes and the label or arguments, to be consistent with the non-polymorphic case. (ocaml-ppx/ocamlformat#1299) (Craig Ferguson) #### Bug fixes + Fix missing parentheses around `let open` (ocaml-ppx/ocamlformat#1229) (Jules Aguillon) eg. `M.f (M.(x) [@attr])` would be formatted to `M.f M.(x) [@attr]`, which would crash OCamlformat + Remove unecessary parentheses with attributes in some structure items: * extensions and eval items (ocaml-ppx/ocamlformat#1230) (Jules Aguillon) eg. the expression `[%ext (() [@attr])]` or the structure item `(() [@attr]) ;;` * `let _ = ...` constructs (ocaml-ppx/ocamlformat#1244) (Etienne Millon) + Fix some bugs related to comments: * after a function on the rhs of an infix (ocaml-ppx/ocamlformat#1231) (Jules Aguillon) eg. the comment in `(x >>= fun y -> y (* A *))` would be dropped * in module unpack (ocaml-ppx/ocamlformat#1309) (Jules Aguillon) eg. in the module expression `module M = (val x : S (* A *))` + Fix formatting of empty signature payload `[%a:]` (ocaml-ppx/ocamlformat#1236) (Etienne Millon) + Fix parenthesizing when accessing field of construct application (ocaml-ppx/ocamlformat#1247) (Guillaume Petiot) + Fix formatting of attributes on object overrides `{< >}` (ocaml-ppx/ocamlformat#1238) (Etienne Millon) + Fix attributes on coercion (ocaml-ppx/ocamlformat#1239) (Etienne Millon) + Fix formatting of attributes on packed modules (ocaml-ppx/ocamlformat#1243) (Etienne Millon) + Fix parens around binop operations with attributes (ocaml-ppx/ocamlformat#1252, ocaml-ppx/ocamlformat#1306) (Guillaume Petiot, Craig Ferguson) + Remove unecessary parentheses in the argument of indexing operators (ocaml-ppx/ocamlformat#1280) (Jules Aguillon) + Retain attributes on various AST nodes: * field set expressions, e.g. `(a.x <- b) [@A]` (ocaml-ppx/ocamlformat#1284) (Craig Ferguson) * instance variable set expressions, e.g. `(a <- b) [@A]` (ocaml-ppx/ocamlformat#1288) (Craig Ferguson) * indexing operators, e.g. `(a.(b)) [@A]` (ocaml-ppx/ocamlformat#1300) (Craig Ferguson) * sequences, e.g. `(a; b) [@A]` (ocaml-ppx/ocamlformat#1291) (Craig Ferguson) + Avoid unnecessary spacing after object types inside records and polymorphic variants, e.g. `{foo : < .. > [@A]}` and `{ foo : < .. > }` (ocaml-ppx/ocamlformat#1296) (Craig Ferguson) + Fix missing parentheses around tuples with attributes. (ocaml-ppx/ocamlformat#1301) (Craig Ferguson) Previously, `f ((0, 0) [@A])` would be formatted to `f (0, 0) [@A]`, crashing OCamlformat. + Avoid emitting `>]` when an object type is contained in an extension point or attribute payload (ocaml-ppx/ocamlformat#1298) (Craig Ferguson) + Fix crash on the expression `(0).*(0)` (ocaml-ppx/ocamlformat#1304) (Jules Aguillon) It was formatting to `0.*(0)` which parses as an other expression. + Preserve empty doc-comments syntax. (ocaml-ppx/ocamlformat#1311) (Guillaume Petiot) Previously `(**)` would be formatted to `(***)`. + Do not crash when a comment contains just a newline (ocaml-ppx/ocamlformat#1290) (Etienne Millon) + Handle lazy patterns as arguments to `class` (ocaml-ppx/ocamlformat#1289) (Etienne Millon) + Preserve cinaps comments containing unparsable code (ocaml-ppx/ocamlformat#1303) (Jules Aguillon) Previously, OCamlformat would fallback to the "wrapping" logic, making the comment unreadable and crashing in some cases. + Fix normalization of attributes, fixing the docstrings in attributes (ocaml-ppx/ocamlformat#1314) (Guillaume Petiot) + Add missing parentheses around OR-patterns with attributes (ocaml-ppx/ocamlformat#1317) (Guillaume Petiot) + Fix spacing inside parens for symbols when the spacing was handled by the englobing exp (ocaml-ppx/ocamlformat#1316) (Guillaume Petiot) + Fix invalid (unparsable) docstrings (ocaml-ppx/ocamlformat#1315) (Guillaume Petiot) When parsing a comment raises an error in odoc, it is printed as-is. + Fix parenthesizing of optional arguments rebound to non-variables, e.g. `let f ?a:(A) = ()` rather than the unparsable `let f ?a:A = ()` (ocaml-ppx/ocamlformat#1305) (Craig Ferguson)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This attempts to resolve #1254.
Note that this changes the parens behaviour in two existing cases. I'd appreciate your thoughts on what the true behaviour should be.