-
Notifications
You must be signed in to change notification settings - Fork 243
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
Initial implementation of swift-format. #1
Conversation
Co-authored-by: Alexander Lash <[email protected]> Co-authored-by: Alfredo Delli Bovi <[email protected]> Co-authored-by: Andrés Tamez Hdz <[email protected]> Co-authored-by: Austin Belknap <[email protected]> Co-authored-by: Harlan Haskins <[email protected]> Co-authored-by: Lauren White <[email protected]> Co-authored-by: Tony Allevato <[email protected]> Co-authored-by: Yuta Saito <[email protected]>
Co-Authored-By: Dave Abrahams <[email protected]>
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 so much for pushing this project! I'm so glad to see it finally pushed and in such great shape!
I have a few comments, but I don't think any of them are critical enough to block merging this. Really excellent work, everyone!
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
public enum SwiftFormatError: Error { |
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.
Nitpick: Docs here
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.
outputStream.write(printer.prettyPrint()) | ||
} | ||
|
||
// TODO: Add an overload of `format` that takes the source text directly. |
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.
Will we start using bugs.swift.org
for swift-format bugs? If so, it'd be nice to file this and link to it in the TODO
.
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.
Yes, we have a swift-format component there now, and I have a ton of things to file 😰
public let fileURL: URL | ||
|
||
/// Indicates whether the file imports XCTest, and is test code | ||
public var importsXCTest: Bool |
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.
Any chance this can be an enum with three cases (or a Bool?
?) It'd be nice to avoid two separate Bool vars.
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.
Updated to use an enum
.
self.importsXCTest = false | ||
self.didSetImportsXCTest = false | ||
self.sourceLocationConverter = SourceLocationConverter(file: fileURL.path, source: sourceText) | ||
let syntax = try! SyntaxParser.parse(source: sourceText) |
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.
If this is going to try!
, we should probably document that sourceText
must be valid source, or convert this to a throw
-ing method.
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 initializer was only used by tests, but I refactored the call sites a bit to remove it completely.
} | ||
|
||
public func visit(_ node: SwitchStmtSyntax) -> SyntaxVisitorContinueKind { | ||
guard let switchIndentation = node.leadingTrivia?.numberOfSpaces else { |
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.
Should this also handle tabs?
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 think this rule can be removed now that we have the whitespace linter implemented; I'll need to verify.
for line in comments.dropFirst(numOfOneSentenceLines) { | ||
let trimmedLine = line.trimmingCharacters(in: .whitespaces) | ||
|
||
if trimmedLine.starts(with: "- Parameters") { |
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.
SourceKit should really have routines for parsing these doc comments directly. If not, we should add them. @nkcsgexi do you think that's possible?
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're planning to use cmark to parse doc comments (and potentially reflow them to fit line limits, since cmark can also re-render them, but that will require some changes to cmark itself to preserve the original markup). I don't know if it would make sense to move that into swift-syntax eventually, though, or to expose it in _InternalSyntaxParser.
/// Format: Enum cases with empty parentheses will have their parentheses removed. | ||
/// | ||
/// - SeeAlso: https://google.github.io/swift#enum-cases | ||
public final class NoEmptyAssociatedValues: SyntaxFormatRule { |
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 think this rule can be deleted since it's no longer valid given some of the SE-155 changes have landed.
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.
/// initializer will yield a lint error. | ||
/// | ||
/// - SeeAlso: https://google.github.io/swift#initializers-2 | ||
public struct UseSynthesizedInitializer: SyntaxLintRule { |
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 may need to be revisited given SE-0242
Sources/swift-format/Run.swift
Outdated
return 1 | ||
} catch { | ||
// Workaround: we're unable to directly catch unknownTokenKind errors due to access | ||
// restrictions. TODO: this can be removed when we update to Swift 5.0. |
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.
Can this be removed?
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.
/// A syntax visitor that delegates to individual rules for linting. | ||
/// | ||
/// This file will be extended with `visit` methods in Pipelines+Generated.swift. | ||
struct LintPipeline: SyntaxVisitor { |
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.
With the way you setup the pipeline it's probably better to switch this to an overridable class by making it a subclass of newly introduced SyntaxVisitorBase
but this can be done as follow-up later on.
No description provided.