Skip to content
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

Jump to bottom

Refactor handling of comments #2371

Merged
merged 63 commits into from Nov 8, 2023
Merged

Refactor handling of comments #2371

merged 63 commits into from Nov 8, 2023

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Jun 6, 2023

This rewrite part of the code that parses and formats comments to remove repeated or inconsistent code.

One function interprets comments: Cmt.decode. It returns a richer type that allow to implement formatting as it was before.
The normalization is changed to use that, no more is_docstring and normalization of indentation is now consistent. This fixes normalization problem for comments parsed as doc and cinaps comments.

Julow added 26 commits May 31, 2023 19:14
@Julow
WIP: Factorize interpretation of comments
7df9213 
Define and centralize how comments are interpreted. The new function is
also used by normalization, which had inconsistent rules before.
@Julow
Fix lost ending space
9caf607
@Julow
Promote
207f471
@Julow
WIP: Rewrite parsing of normal and asterisk prefixed comments
86ff2e4 
A few regressions
@Julow
Test 'error4' requires one more iteration
f5cce1a
@Julow
Fix added newline in cinaps comments
032f6c3
@Julow
Strip trailing spaces of cinaps comments
124a89b
@Julow
Promote
2d0930d
@Julow
Strip heading/trailing empty lines
b207454 
This fixes two instances of unstable formatting but might be a
regression when `wrap-comments=false`.
@Julow
Small cleanup in docstring fmt function
e8875b5
@Julow
Fix incorrect unindenting of cinaps comments
c42280a
@Julow
Normalize comments in code in comments
4e4ef13
@Julow
Revert "Normalize comments in code in comments"
7e9e6ab 
This reverts commit 70e5752b06fdef97e3c729d3bbfed14888ece62a.
@Julow
Fix unindenting when first line is empty
6061527
@Julow
Don't unindent doc comments
520a35c 
The indentation of doc comments is significative for verbatim blocks.
The decision of parsing a regular comment as doc must be done before
decoding a comment.

Regressions are due to test cases previously crashing finally being run.
@Julow
Revert "Test 'error4' requires one more iteration"
2095965 
This reverts commit f5cce1a.

No longer the case.
@Julow
Trim trailing empty lines and whitespaces
606447a
@Julow
Preserve empty trailing lines in doc comments
4572014
@Julow
Restore formatting of cinaps comments
3468395
@Julow
Preserve pro/epi break on comments as doc
aa80541
@Julow
Fix regressions on unwrapped comments
364fc75
@Julow
Fix parsing of asterisk prefixed comments
e009001
@Julow
Restore break before preceeding multi-line comments
7bff959 
This break was removed in previous commits
@Julow
Preserve leading/trailing newlines in unwrapped comments
f8719ba
@Julow
Tests: Remove no longer necessary --max-iter
69f2a8e
@Julow
Tests: Remove empty .err files
5a56bf1
Julow
Julow commented Jun 6, 2023
View reviewed changes
lib-rpc-server/ocamlformat_rpc.ml Outdated Show resolved Hide resolved
test/passing/tests/break_separators-after_docked.ml.ref Show resolved Hide resolved
test/passing/tests/doc_comments-after.ml.ref Show resolved Hide resolved
test/passing/tests/js_source.ml.ocp Show resolved Hide resolved
test/passing/tests/sequence-preserve.ml.ref Show resolved Hide resolved
test/passing/tests/wrap_comments.ml.ref Outdated Show resolved Hide resolved
Julow added 3 commits June 6, 2023 15:16
@Julow
Fix parsing and printing of header-like comments
37bf3f0 
Allow indented lines with no asterisks and trailing newline in asterisk
prefixed comments.

A trailing newline results in the star of the closing token to be
aligned with the asterisks.
@Julow
Update changes
8bf52f2
@Julow
Fix parsing of asterisk prefixed comments too open
cf878da
@Julow
Copy link
Collaborator Author

Julow commented Jun 22, 2023

The changes caused by this PR are reasonable to me: the original placement of *) is respected more often, comments that shouldn't be wrapped are not.
I don't see any regression in test_branch on projects that were previously formatted with the default profile.
I would merge this in its current state.

Any opinion @jberdine @gpetiot ?

This was referenced Oct 10, 2023
Merged
Merged
@Julow Julow mentioned this pull request Oct 11, 2023
Merged
@Julow Julow mentioned this pull request Oct 26, 2023
Closed
@Julow
Copy link
Collaborator Author

Julow commented Oct 26, 2023

Closes #2468

@tdelvecchio-jsc tdelvecchio-jsc mentioned this pull request Nov 2, 2023
Closed
@jberdine
Copy link
Collaborator

jberdine commented Nov 3, 2023

I had another quick look at this, do I understand correctly that in the current state of this PR there are still a number of regressions where lines are now wrapped before they reach the margin, and a number of regressions where the margin is now exceeded? The plan is to resolve these before merging this, right?

@Julow
Copy link
Collaborator Author

Julow commented Nov 3, 2023

I'm still investigating this issue but I'm now considering merging without it fixed. The issue seems rare and I'm clueless on how to fix it.

Julow and others added 7 commits November 6, 2023 12:37
@Julow
Merge branch 'MAIN' into cmts-factor
20f4fb8
@Julow
test: Add non-stabilizing comment
c86b00c
@Julow
Fix non-stabilizing comment
907d70a 
Remove the forced line break before a multi-line comment. The asymmetry
of this forced line break allowed comments to be move back and forth
between being attached to after `f` or before `a`.

This adds regressions.
@gpetiot
Fix regression (break introduced before the end of a paragraph)
a400535
@Julow
Don't trim first line of asterisk prefixed comment
a0c9aae
@Julow
Add test case from ocaml-ppx#2469
5f5532e
@Julow
test: Fix spacing in cinaps.ml
86e12c3 
Consecutive comments with no empty line in between are not formatted.
gpetiot
gpetiot approved these changes Nov 8, 2023
View reviewed changes
Copy link
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Splitting the decoding/formatting makes the code easier to maintain indeed.
The only changes on the tests are improvements, so let's merge this one :)

@Julow
Copy link
Collaborator Author

Julow commented Nov 8, 2023

The unexplained break regression is fixed and the new cases where the margin is exceeded is due to asterisk prefixed comments that do not wrap. This is ready to be merged!

@Julow Julow merged commit d0a28cf into ocaml-ppx:main Nov 8, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jberdine jberdine jberdine left review comments

@gpetiot gpetiot gpetiot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Julow @jberdine @gpetiot