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

Add location of invalid docstring in warning messages #1529

Merged
merged 7 commits into from
Dec 2, 2020

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Nov 4, 2020

Now the invalid docstrings are not reported as warnings anymore but make ocamlformat exit with code 1. I made it the default behavior but we can also choose to hide it behind a new option. (outdated)

@gpetiot gpetiot requested review from emillon and Julow November 4, 2020 18:37
@gpetiot gpetiot added this to the 0.17.0 milestone Nov 5, 2020
Copy link
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.

I wonder if this can break the workflow of some people ? For example, when enabling parse-docstrings on a project that contains a lot of comments that don't parse, every comments will now have to be fixed at the same time.
I suspect this breaks parse-wyc, we should keep the warning in this case.

@gpetiot gpetiot changed the title Fail on invalid docstrings Add location of invalid docstring in warning messages, checks are made when parsing the file Nov 25, 2020
@gpetiot gpetiot requested a review from Julow November 25, 2020 16:27
@gpetiot
Copy link
Collaborator Author

gpetiot commented Nov 25, 2020

I reverted some of these changes, so ocamlformat still doesn't fail on invalid docstrings, all the warnings are printed during the parsing phase instead of in the middle of Fmt_ast, and missing locations are added.

Copy link
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.

The cost is to parse each docstrings twice. It's not a huge cost but what are the advantages ?
Showing the location is nice. (grepping for comments is not the best UI)

@gpetiot
Copy link
Collaborator Author

gpetiot commented Nov 25, 2020

True, it made more sense when we would fail on invalid docstrings at the beginning, well I find it slightly better to group the checks when parsing the file. We could memoize the parsed docstrings if the cost is too visible to users

@gpetiot gpetiot changed the title Add location of invalid docstring in warning messages, checks are made when parsing the file Add location of invalid docstring in warning messages Nov 27, 2020
@gpetiot
Copy link
Collaborator Author

gpetiot commented Nov 27, 2020

I moved the checks back where they were to reduce the diff.

@gpetiot gpetiot requested a review from Julow December 1, 2020 18:26
Copy link
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.

Looks good.

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.

3 participants