Upgrade the DontRepeatTypeInStaticProperties rule. #777
Conversation
|
Should be a partial fix for #771, since it upgrades one rule only. Interested in feedback on if there is a better way to do this before upgrading any other rules. |
shawnhyam
force-pushed
the
upgrade-dont-repeat-type-in-static-properties
branch
2 times, most recently
from
9cdffee
to
a700197
Compare
Sources/SwiftFormat/Rules/DontRepeatTypeInStaticProperties.swift
Outdated
Show resolved
Hide resolved
|
@shawnhyam Instead of merging |
shawnhyam
force-pushed
the
upgrade-dont-repeat-type-in-static-properties
branch
from
1280cae
to
7496b30
Compare
|
@allevato I was wondering if you had any thoughts on this, especially the changes to the pipeline generator (so that |
| extension Syntax { | ||
| /// Returns the name of the immediately enclosing type of this node if there is one, | ||
| /// otherwise nil. | ||
| fileprivate var typeName: String? { |
There was a problem hiding this comment.
The name of this property feels a little misleading; when I saw you accessing the typeName of a VariableDeclSyntax node above, it read to me like it was accessing the type of the variable. Can we call it containingDeclName so that the nature of the traversal is clear at the call site?
| @@ -16,6 +16,7 @@ import SwiftSyntax | |||
| /// | |||
| /// This rule does not apply to test code, defined as code which: | |||
| /// * Contains the line `import XCTest` | |||
| /// * The function is marked with `@Test` attribute | |||
There was a problem hiding this comment.
Is this noise from another PR that was merged in?
There was a problem hiding this comment.
Is this noise from another PR that was merged in?
This was because I ran generate-swift-format, and there have been some manual edits to the generated files. So I made the code edits necessary to keep the generated file the same.
| diagnose(.removeTypeFromName(name: varName, type: bareTypeName), on: identifierPattern) | ||
| } | ||
| let varName = identifierPattern.identifier.text | ||
| if varName.contains(bareTypeName) { |
There was a problem hiding this comment.
Now that I'm getting another look at this, I wonder if we should update this to be hasSuffix, since the diagnostic explicitly talks about suffixes.
There was a problem hiding this comment.
Good question... we wouldn't catch plurals (which we don't mention or have tests for), but there are likely false positives with the current logic too.
public actor Cookie {
// cases that are caught with .contains but not with .hasSuffix
static let chocolateChipCookies: Int
static let rawCookieDough: Int
}
- Using the visit method to examine all the variable declarations should allow specific decls to be ignored. - Modify the PipelineGenerator to produce output matching the current generated files.
shawnhyam
force-pushed
the
upgrade-dont-repeat-type-in-static-properties
branch
from
7496b30
to
3155cb5
Compare
|
Thanks for fixing this! |
visitmethod to examine all the variable declarations should allow specific decls to be ignored.visitPostmethods