-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Update file_name
rule to match fully-qualified names of nested types
#5841
base: main
Are you sure you want to change the base?
Update file_name
rule to match fully-qualified names of nested types
#5841
Conversation
@@ -21,6 +22,8 @@ final class FileNameRuleTests: SwiftLintTestCase { | |||
rule = try FileNameRule(configuration: ["suffix_pattern": suffixPattern]) | |||
} else if let nestedTypeSeparator { | |||
rule = try FileNameRule(configuration: ["nested_type_separator": nestedTypeSeparator]) | |||
} else if let fullyQualified { | |||
rule = try FileNameRule(configuration: ["fully_qualified": fullyQualified]) |
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.
Open to other names here such as require_fully_qualified
, require_fully_qualified_names
, etc.
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 like the last one most.
This PR is aimed to address Issue realm#5840. It does the following: 1. Allows the `file_name` rule to match nested types when using fully-qualified names, meaning naming the following file `Nested.MyType.swift` is no longer a violation: ``` // Nested.MyType.swift enum Nested { struct MyType { } } ``` 2. Introduces a new option `fully_qualified` to have the `file_name` rule enforce using fully-qualified names, meaning naming the above file `MyType.swift` instead of `Nested.MyType.swift` would become a violation where it wasn't before (naming the file `Nested.swift` would still not be a violation).
9399bc9
to
904bad2
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.
I like the extension of the rule. Thanks for the contribution!
Please find a few remarks as separate inline comment.
Source/SwiftLintBuiltInRules/Rules/Idiomatic/FileNameRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/FileNameRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Idiomatic/FileNameRule.swift
Outdated
Show resolved
Hide resolved
extension StructDeclSyntax: TypeNameCollectible {} | ||
extension TypeAliasDeclSyntax: TypeNameCollectible {} | ||
extension EnumDeclSyntax: TypeNameCollectible {} | ||
extension ProtocolDeclSyntax: TypeNameCollectible {} |
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.
That's a lot of overhead only to spare one more method. Honestly, I'd prefer to go with a visit(name:String)
and a visit(node:NamedDeclSyntax)
. The latter can just redirect to the first after extracting the name. And visit(_: ExtensionDeclSyntax)
can call it directly.
names.insert(name) | ||
} | ||
|
||
ancestorNames.push(node.typeName) |
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.
ancestorNames.push(node.typeName) | |
ancestorNames.push(name) |
This PR is aimed to address Issue #5840. It does the following:
file_name
rule to match nested types when using fully-qualified names, meaning naming the following fileNested.MyType.swift
is no longer a violation:fully_qualified
to have thefile_name
rule enforce using fully-qualified names, meaning naming the above fileMyType.swift
instead ofNested.MyType.swift
would become a violation where it wasn't before (naming the fileNested.swift
would still not be a violation).