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.

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

Better ignore doc comment #2456

Merged
merged 2 commits into from
Oct 9, 2023
Merged

Better ignore doc comment #2456

merged 2 commits into from
Oct 9, 2023

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented Oct 5, 2023

Fix #2408
Floating doc comment were not ignored when using --no-comment-check, only attached doc-comment were.

This change can result in empty Ptop_def [] which are now ignored while mapping.

For the record, I've tested this PR on the all sources inside opam-respository

find ../all-sources -name "*.ml" -type f -o -name "*.mli" -type f | parallel --progress _build/default/bin/ocamlformat/main.exe --enable-outside-detected-project -q --no-comment-check --no-version-check --ignore-invalid-option > /dev/null

I don't see any Ast changed bug anymore

@Julow
Copy link
Collaborator

Julow commented Oct 6, 2023

I'm not sure we should ignore docstrings while checking. They are part of the AST and loosing or moving one would be catastrophic. Perhaps the normalization function could be improved instead ?

What's your use case for --no-comment-check ?

@hhugo
Copy link
Collaborator Author

hhugo commented Oct 6, 2023

The purpose of --no-comment-check is to ignore comments and doc-comments !

Here is what the ocamlformat doc says

Control whether to check comments and documentation comments. Unsafe to turn off. May be set in .ocamlformat. The flag is set by default.

The flag was introduced for debug/dev purposes so that one case focus on Ast changed errors. I use this in #985

@Julow
Copy link
Collaborator

Julow commented Oct 6, 2023

Doc comments are part of the AST and I don't consider them problematic like comments. I think doc comments issues are of higher priority than other Ast changed errors.
I'd prefer to fix these first rather than to ignore them.
What do you think?

@hhugo
Copy link
Collaborator Author

hhugo commented Oct 6, 2023

Please looks at what's done currently. Please look at the documentation for this option. This option is not supposed to be used by regular users. It currently already ignores some doc-comments. This PR just make the behavior consistent.

I don't really understand your pushback on this developers option.

This change allows to run ocamlformat on files triggering warning 50 and spot Changed ast without the noise caused by comments.

@hhugo
Copy link
Collaborator Author

hhugo commented Oct 7, 2023

Also see PR introducing the feature #259

@hhugo
Copy link
Collaborator Author

hhugo commented Oct 7, 2023

This PR is restoring the feature introduced in #259 that was altered at some point later

@hhugo
Copy link
Collaborator Author

hhugo commented Oct 8, 2023

The regression was introduced by #1672
The following statement was invalid

or double checking things like ignore_doc_comments (checked by the attributes method anyway), maybe these were considered to be optimizations but I didn't observe any slowdown;

@Julow
Copy link
Collaborator

Julow commented Oct 9, 2023

Alright, you convinced me it's useful, let's merge.

@Julow Julow merged commit b790cc4 into main Oct 9, 2023
7 of 9 checks passed
@gpetiot gpetiot deleted the ignore-comment branch October 9, 2023 08:27
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.

Feature request: improve the behavior of --no-comment-check
2 participants