Skip to content

Attributes now stay where the user put them#2451

Merged
Julow merged 13 commits intoocaml-ppx:mainfrom
EmileTrotignon:ext_attrs_module
Sep 27, 2023
Merged

Attributes now stay where the user put them#2451
Julow merged 13 commits intoocaml-ppx:mainfrom
EmileTrotignon:ext_attrs_module

Conversation

@EmileTrotignon
Copy link
Copy Markdown
Collaborator

@EmileTrotignon EmileTrotignon commented Sep 22, 2023

module [@attr] M = struct end

used to be formatted into

module = struct end [@@attr]

This PR make it so that the user can choose between the two syntaxes, ocamlformat will not change anything.

It contains the first commit of #2247

Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Very nice!

Comment thread lib/Ast.ml Outdated
Comment thread lib/Fmt_ast.ml Outdated
Comment thread test/passing/tests/attributes.ml
Comment thread test/passing/tests/js_source.ml.ocp
Comment thread lib/Fmt_ast.ml Outdated
Comment thread vendor/parser-extended/docstrings.ml Outdated
Comment thread vendor/parser-extended/parser.mly Outdated
Comment thread vendor/parser-extended/parser.mly
Comment thread test.ml Outdated
Comment thread vendor/parser-extended/parser.mly Outdated
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Nice work :) A few tweaks and it's ready to merge.

Comment thread lib/Ast.ml Outdated
Comment thread lib/Ast.ml Outdated
Comment thread lib/Ast.ml Outdated
Comment thread vendor/parser-extended/parser.mly
Comment thread CHANGES.md Outdated
- JaneStreet profile: doesn't align infix ops with open paren (#2204, @gpetiot)
- Re-use the type let_binding from the parser instead of value_binding, improve the spacing of let-bindings regarding of having extension or comments (#2219, @gpetiot)
- The `ocamlformat` package now only contains the binary, the library is available through the `ocamlformat-lib` package (#2230, @gpetiot)
- Modules attributes are now formatted right after the `module` keyword, instead of at the end of the module definition (#2247, @emiletrotignon)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This makes me think that the attributes are moved, which is not the case. Also, future patches will not be about modules.
We used the word "preserve" for this in the past.
Perhaps it can be clearer with less explanation and a small code snippet ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think in future patches where we preserve more attributes we will edit the line instead of adding a new one, I updated with something that fits what this PR does.

Comment thread lib/Fmt_ast.ml Outdated
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

This is good to go, thanks !

There's a small regression related to #2450 but it's not worth blocking on that.

@Julow Julow merged commit bc91e42 into ocaml-ppx:main Sep 27, 2023
Julow pushed a commit that referenced this pull request Oct 5, 2023
This fixes a bug that was introduced in commit bc91e42 (pr #2451)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants