-
Notifications
You must be signed in to change notification settings - Fork 228
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
Generate rule docs automatically #615
Generate rule docs automatically #615
Conversation
aa32253
to
8a939da
Compare
// | ||
// This source file is part of the Swift.org open source project | ||
// | ||
// Copyright (c) 2014 - 2023 Apple Inc. and the Swift project authors |
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.
Someone told me to put just 2023 if the file is new.
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation |
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.
Honest question: why do we need Foundation
in our generators?
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.
write(into:)
takes a FileHandle
argument.
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.
So technically we could just write into a file differently, but we just don't aim to support environments where Foundation is not available yet?
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 can't think of any platforms that don't have Foundation that I'd want to run generate-pipeline on.
The rest of swift-format also uses Foundation, so I don't know why I'd want to single out generate-pipeline specifically.
89390d0
to
8a939da
Compare
- This commit adds a `generate-pipeline` generator `RuleDocumentationGenerator` that extracts each rule DocC comment, and writes it into `Documentation/RuleDocumentation.md`. - In `Documentation/Configuration.md`, I've added a paragraph on rules configuration that links to the new auto-generated docs file, and also tells users that they can run `swift-format dump-configuration` to check out the list of available rules.
8a939da
to
1556e82
Compare
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.
Thanks, this looks really good! A few suggestions but after that I think this is good to go and the iterate on afterwards.
[Configuration](Documentation/Configuration.md). All of these rules can be | ||
applied in the linter, but only some of them can format your source code | ||
automatically. | ||
|
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.
Could you generate an unordered list here that would act as a table of contents, linking to each of the headers below?
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.
Done!
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation |
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.
write(into:)
takes a FileHandle
argument.
|
||
/// Takes the DocC comment of the rule and strip `///` from the beginning of each line. | ||
/// Also removes empty lines with under 4 characters. | ||
private func ruleDescription(for rule: RuleCollector.DetectedRule) -> String { |
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.
We already have more general code for parsing doc comments in DocumentationCommentText
. You could use that here instead (it's under @_spi
, but that's fine).
generate-pipeline
already imports SwiftFormat
because it actually pokes at some of the runtime properties of the rules. It's kind of weird, but it means you don't have to worry about introducing that dependency.
- Using `DocumentationCommentText` via `@_spi(Rules)` instead of our own implementation of rule description extracted from trivia. - Added table of contents with all the available rules linked to their respective blocks.
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.
Ready for another pass, @allevato!
[Configuration](Documentation/Configuration.md). All of these rules can be | ||
applied in the linter, but only some of them can format your source code | ||
automatically. | ||
|
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.
Done!
@@ -18,6 +18,7 @@ import SwiftSyntax | |||
/// structural organization. It automatically handles trimming leading indentation from comments as | |||
/// well as "ASCII art" in block comments (i.e., leading asterisks on each line). | |||
@_spi(Testing) | |||
@_spi(Rules) |
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.
Added additional SPI here because using @_spi(Testing)
just felt janky.
@@ -23,6 +23,10 @@ final class RuleCollector { | |||
/// The type name of the rule. | |||
let typeName: String | |||
|
|||
/// The description of the rule, extracted from the rule class or struct DocC comment | |||
/// with `DocumentationCommentText(extractedFrom:)` | |||
let description: String? |
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.
Optional because DocumentCommentText()
may return nil, but that's fine for us.
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
import Foundation |
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.
So technically we could just write into a file differently, but we just don't aim to support environments where Foundation is not available yet?
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.
Thank you for this! I'm really happy with how it turned out and it'll be a big help keeping the rule documentation updated going forward.
If you're looking for something else to do 😁 , it would be nice to get coverage for the rest of the fields in Configuration
and the types associated with them. That's a bit more work though, because we're not already collecting the decls today. But basically we'd just want to scan Configuration.swift for all the fields that are public var
s, and then do the same for the other types defined in that file when we have config fields that are themselves complex values.
@allevato, thank you for reviewing this and guiding me through it! ❤️ I'm happy to help more. I was curious about trying to convert the Documentation directory to a DocC site first, and maybe even recording that working session / streaming it to get more people comfortable with DocC for their projects. That's likely some time next week. Do you think it would be okay to convert to DocC first, and then add more documentation coverage in Configuration, as well as in code otherwise? |
I think that all sounds fantastic! The public API for swift-format (everything in the API subfolder) is pretty small so getting DocC support up and running should hopefully be fairly straightforward. And I love the idea of recording your work session for the community. If we can only get I wonder if, instead of scanning the source, we could consume the symbol graphs that DocC uses to generate that alternate form instead... I haven't used DocC much myself, so I'm interested to see what you come up with! |
…peline/rule-documentation Generate rule docs automatically
Summary
dump-configuration
with inline comments to explain each rule #606, adds documentation for the list of available rules inDocumentation
directory.Changes
generate-pipeline
generatorRuleDocumentationGenerator
that extracts each rule DocC comment, and writes it as a table intoDocumentation/RuleDocumentation.md
.Documentation/Configuration.md
, I've added a paragraph on rules configuration that links to the new auto-generated docs file, and also tells users that they can runswift-format dump-configuration
to check out the list of available rules.How to review this
RuleDocumentation.md
.TODOs