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

Format comments of the form (*= xx *) as docstrings when parse-docstrings is set #1810

Closed
wants to merge 2 commits into from

Conversation

gpetiot
Copy link
Collaborator

@gpetiot gpetiot commented Sep 16, 2021

This is more of a RFC, based on the discussion started in #1804, as I sitll think removing wrap-comments in the long run is a good idea.
Should replace #1747

The idea is:

  • if wrap-comments is set, it has the priority (until it is removed), so the comments are wrapped (same behavior as main branch)
  • if parse-docstrings is set, parse comments as docstrings if they are valid docstrings, otherwise wrap each paragraph
  • otherwise wrap each paragraph

Running test_branch.sh with parse-docstrings=true shows that a lot of comments should be fixed, but there is some issue with the fallback, characters are not properly escaped, I didn't have time to have a look yet.

I didn't like the idea of introducing a new syntax to format a comment as docstring, as maybe some people will ask us later to keep maitaining this syntax if we want to remove it. To me it sounds good to have it enabled with parse-docstrings as long as the paragraph formatting is a good fallback.

For the record, wrap-comments and parse-docstrings are both disabled by default on all profiles.

What do you think?

cc @ceastlund as this is a feature you have been interested in

@gpetiot gpetiot marked this pull request as ready for review September 21, 2021 07:00
@gpetiot gpetiot force-pushed the format-comments-as-docstrings branch from a258c6b to eb421c7 Compare September 22, 2021 11:49
@jberdine
Copy link
Collaborator

I think that for comments that a user intends to be parsed as a docstring, if parsing fails, we need to give an error or warning. The syntax of docstrings is far too undocumented and unfamiliar to silently swallow parse failures. Additionally, the behavior of wrapping paragraphs when parsing fails is in many cases only a little different, making it very hard indeed for users to know what is going on.

So since:

  1. not all comments can be expected to parse as docstrings
  2. parse failures for comments expected to parse as docstrings must be reported
    then it follows that:
  3. there must be some way for users to indicate the expectation of whether a comment is expected to parse as a docstring

Currently, 3 is achieved be restricting the comments that are attempted to be parsed to (** ... *) comments.

While I am definitely in favor of parsing more comments as docstrings, and of eliminating the separate wrap comments option, it seems necessary to introduce a comment form to indicate that the comment should be parsed as a docstring, and report an error/warning if it does not parse.

Setting aside implementation issues for the moment, are there objections to using the comment form (*= ... *), say, for comments that should be parsed as docstrings, just as (** ... *) are now, but able to appear anywhere and not consumed by other tools like odoc. Then all other comments could uniformly be left unchanged (apart from adjusting indentation). This would also enable removing the special handling of banner comments IIUC.

@gpetiot gpetiot force-pushed the format-comments-as-docstrings branch from 1605dbf to a725e53 Compare October 5, 2021 12:13
@gpetiot
Copy link
Collaborator Author

gpetiot commented Oct 5, 2021

That sounds like a good idea to me, I've updated the PR to see what it would look like. Having to maintain a new syntax can indeed make the transition smoother for users. So far this new feature is relying on the parse-docstrings flag being set (for ast identity check, but that's easy to modify the check I just went through the faster lane).
Let me know what you think!

So after this PR the next step would be to make all other comments verbatim? and thus deprecating the wrap-comments option? (so deprecate in 0.20.0, then removing in the subsequent release)

@gpetiot gpetiot force-pushed the format-comments-as-docstrings branch from 7a00daa to 6ace8f0 Compare October 5, 2021 15:47
@gpetiot gpetiot changed the title Format comments as docstrings when parse-docstrings is set, wrap each paragraph otherwise Format special (*= xx *) comments as docstrings when parse-docstrings is set Oct 5, 2021
@gpetiot gpetiot force-pushed the format-comments-as-docstrings branch from 6ace8f0 to 3b295ac Compare October 8, 2021 21:24
@gpetiot gpetiot changed the title Format special (*= xx *) comments as docstrings when parse-docstrings is set Format comments of the form (*= xx *) as docstrings when parse-docstrings is set Oct 8, 2021
@samoht
Copy link
Contributor

samoht commented Oct 9, 2021

While I am definitely in favor of parsing more comments as docstrings, and of eliminating the separate wrap comments option, it seems necessary to introduce a comment form to indicate that the comment should be parsed as a docstring, and report an error/warning if it does not parse.

Would it be possible to do the reverse? E.g. by default parse every comment as a docstring, but add an escape hatch (mainly for ASCII arts). In that case (*= (or anything else) would be used for the escape hatch only. When I look at the comments that I write, most of them are just normal docstrings, so I'd like to optimize for this use-case first.

@gpetiot
Copy link
Collaborator Author

gpetiot commented Nov 8, 2021

It sounds reasonable to me. Following the discussion of #1804 we could then:

  • add the (*= ... *) syntax to preserve comments (verbatim) no matter the value of parse-docstrings and wrap-comments (we can add it for 0.20.0)
  • document the accepted syntax of doc-comments (before 0.21.0)
  • deprecate wrap-comments (already planned for 0.21.0)
  • remove wrap-comments (already planned for 0.22.0), we can make comments wrapped by default and parsed as docstrings

So far it's not really planned to deprecate parse-docstrings this option would still impact doc-comments, but since the formatting of comments involves wrap-comments as well (in case this is not a valid doc-comment) then it makes sense to do the same for doc-comments and comments. So I think it's better to deprecate then remove parse-docstrings at the same time as wrap-comments.

Thoughts @jberdine @samoht @Julow ?

@gpetiot
Copy link
Collaborator Author

gpetiot commented Feb 28, 2022

Replaced by #2028

@gpetiot gpetiot closed this Feb 28, 2022
@gpetiot gpetiot deleted the format-comments-as-docstrings branch February 28, 2022 17:43
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.

4 participants