-
Notifications
You must be signed in to change notification settings - Fork 284
FormatWriter: format multiline docstrings #1987
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
Conversation
olafurpg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exciting! A few more test cases that I think would be nice to cover.
- code formatted with backticks (single line and multiline). Even if it’s not official scaladoc markup syntax I’ve seen it used several places and it renders as expected in some cases.
- quoted strings (single and double), whether they get line wrapped or not we should document the behavior in the tests.
- URLs followed by a white space and period. Just make sure not to remove the space.
- HTML markup, white space inside HTML is significant so we should consider leaving it unchanged. Otherwise we may want to consider writing a full blown XML pretty printer.
scalafmt-core/shared/src/main/scala/org/scalafmt/config/Docstrings.scala
Outdated
Show resolved
Hide resolved
kitbellew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- code formatted with backticks (single line and multiline). Even if it’s not official scaladoc markup syntax I’ve seen it used several places and it renders as expected in some cases.
- quoted strings (single and double), whether they get line wrapped or not we should document the behavior in the tests.
- URLs followed by a white space and period. Just make sure not to remove the space.
- HTML markup, white space inside HTML is significant so we should consider leaving it unchanged. Otherwise we may want to consider writing a full blown XML pretty printer.
will do.
scalafmt-core/shared/src/main/scala/org/scalafmt/config/Docstrings.scala
Outdated
Show resolved
Hide resolved
Create a class and move the existing method there. We will subsequently add methods to format and wrap docstrings.
Added a test for single backquotes. In order to support triple backquotes, we need to modify the parser in scalameta; however, I am not sure how valid or useful that would be, as Intellij doesn't support them, and they don't seem to be supported in any scaladoc documentation (https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html#markup, http://keithpinson.github.io/Scaladoc-HOWTO/api/docSample/syntax.html)
Added both; there's no special handling for now, they are wrapped as any other space-separated words.
Added; no special handling, space separates the period into a separate word.
Added a test but I doubt this is what you had in mind. To support, we need to extend the scaladoc parser in scalameta; there's Ideally, if we are to support html, we'd have to parse I'd address this in a subsequent version, as that's highly non-trivial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree XML parsing is non-trivial and it's not clear whether it's worth the investment. Just having tests to document the current behavior is a good start.
It might be worth mentioning in the docs that HTML tags are formatted just like normal text, which may produce unexpected output for tables and other large HTML elements.
kitbellew
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth mentioning in the docs that HTML tags are formatted just like normal text, which may produce unexpected output for tables and other large HTML elements.
i have some warning to that effect, please see below; should it be revised?
| > This functionality is generally limited to | ||
| > [standard scaladoc elements](https://docs.scala-lang.org/overviews/scaladoc/for-library-authors.html) | ||
| > and might lead to undesirable results in corner cases; | ||
| > for instance, the scaladoc parser doesn't have proper support of embedded HTML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olafurpg would you like to amend this statement to say it will be treated as normal text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect, i will merge.
separately, i will add table support to scalameta and then propagate it here. does this sound reasonable: https://www.scala-lang.org/blog/2018/10/04/scaladoc-tables.html
|
Hi even after taking latest release i am still seeing issue #1387? Do I need to something more? |
please submit an issue providing all of the information, including a reproducible example. |
Thanks for replying. Sure will raise one soon. |
Now that scalameta in 4.3.13 supports parsing of docstrings, let's use that and format.
Fixes #1387.
Fixes #1887.