-
Notifications
You must be signed in to change notification settings - Fork 242
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 a StandardizeDocumentationComments rule #959
base: main
Are you sure you want to change the base?
Add a StandardizeDocumentationComments rule #959
Conversation
This adds a formatting rule that rewraps and restructures documentation comments to be in a consistent order and format. Other comments are left unchanged.
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 so good. 😍
I'll wait to do a deeper review when things are more settled, but in terms of direction, this is exactly the kind of behavior I wanted to see.
// different for different node types (e.g. an accessor has a `body`, while an | ||
// actor has a `memberBlock`). | ||
|
||
public override func visit(_ node: AccessorDeclSyntax) -> DeclSyntax { |
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.
Does the Swift compiler or DocC process comments on accessor independently of those
on the parent property/subscript?
Likewise for some of the other declarations included here, like deinit
and extension
.
I suppose it doesn't hurt to include them here so we shouldn't necessarily exclude them, but I'm more curious if there are generally user expectations around these specific ones.
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.
Ah, not sure! I just grabbed the list of decls from... here. I'll try it out and see what happens – we could probably just include nominal declarations.
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.
Removed this and the other non-documentable declaration types 👍🏻
This behavior matches DocC, which treats all outline list items within a parameter's documentation as plain markup, rather than as special fields.
This reverts commit 932e7a4.
Instead of parsing nested parameter documentation differently, which can have source-breaking effects, capture all the unfiltered body nodes when initializing a `DocumentationComment` from markup. This lets us choose whether to use the filtered body (at the top level of a doc comment) or the full list of body nodes (within a parameter's nested documentation).
Potential controversies in documentation formatting:
|
I think these are fine; after all, the rule is named Standardize. Of those you listed, the first and last don't bother me. Not being able to distinguish between inline and reference links isn't ideal, but if we're limited by the underlying library, there's probably not much we can do. Another issue just occurred to me, which is a little trickier to solve. The structure of the formatter is such that whitespace/indentation isn't handled until the pretty-printing phase, after all the syntax walking rules have done their operations. That means that the calculations you make in that rule may not be the correct ones. Consider this example:
This will reflow the doc comment based on the difference between the line width and its leading trivia, which gives is two more spaces than it should have. When the pretty printer indents that function and its comment, it could exceed the line width, and then formatting it again would fix it. This means that if we want it to be correct on the first format, rendering the doc comment back to source would have to happen in the pretty printer, once we know what the actual indentation is going to be. We could do everything but rewrapping in the rule that you have now, with the drawback being that we'd end up parsing every doc comment twice (once in the rule pipeline to standardize the Markdown structure, then again to get the Markdown doc that we re-render). To avoid that, we could do both in the pretty printer instead. Fortunately, the way the pretty printer works is that the At that point in the code, we don't have access to what kind of node the doc comment was attached to. We could augment the |
This adds a formatting rule that rewraps and restructures documentation comments to be in a consistent order and format. Other comments are left unchanged. The standardization enforces the following rules:
///
-prefixed.The change needs more tests, especially for parameters with rich documentation (since most of that will get dropped on the floor right now). There are also some slight issues in the way swiftlang/swift-markdown does line-wrapping, particularly around inline code. I've opened a fix for those issues here: swiftlang/swift-markdown#215
In addition to the tests, you can see the result of this rule on ArgumentParser in this branch:
https://github.com/apple/swift-argument-parser/compare/standardized-docs