Skip to content
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 ignore_codingkeys parameter to nesting #5650

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

braker1nine
Copy link
Contributor

Resolves #5641

This very explicitly targets enum's named CodingKeys which conform to CodingKey. To provide an exception for Codable users who have to provide custom keys.

OK ✅

struct Outer {
    struct Inner: Codable {
        let identifier: Int
        enum CodingKeys: String, CodingKey {
            case identifier = "id"
        }
    }
}

Not OK ❌

struct Outer {
    struct Inner {
        let identifier: Int
        enum SomeKeys: String, CodingKey {
            case identifier = "id"
        }
    }
}

This only returns true when the name of the enum is
CodingKeys and it conforms to CodingKey
This prevents nested CodingKeys (which may be necessary
for functioning Codable conformance) from triggering
nesting violations
@SwiftLintBot
Copy link

SwiftLintBot commented Jul 2, 2024

17 Messages
📖 Linting Aerial with this PR took 0.91s vs 0.93s on main (2% faster)
📖 Linting Alamofire with this PR took 1.22s vs 1.23s on main (0% faster)
📖 Linting Brave with this PR took 7.31s vs 7.3s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 4.09s vs 4.02s on main (1% slower)
📖 Linting Firefox with this PR took 10.36s vs 10.38s on main (0% faster)
📖 Linting Kickstarter with this PR took 9.21s vs 9.2s on main (0% slower)
📖 Linting Moya with this PR took 0.52s vs 0.52s on main (0% slower)
📖 Linting NetNewsWire with this PR took 2.5s vs 2.5s on main (0% slower)
📖 Linting Nimble with this PR took 0.75s vs 0.74s on main (1% slower)
📖 Linting PocketCasts with this PR took 7.77s vs 8.01s on main (2% faster)
📖 Linting Quick with this PR took 0.43s vs 0.42s on main (2% slower)
📖 Linting Realm with this PR took 4.64s vs 4.62s on main (0% slower)
📖 Linting Sourcery with this PR took 2.29s vs 2.31s on main (0% faster)
📖 Linting Swift with this PR took 4.44s vs 4.52s on main (1% faster)
📖 Linting VLC with this PR took 1.24s vs 1.22s on main (1% slower)
📖 Linting Wire with this PR took 17.12s vs 17.32s on main (1% faster)
📖 Linting WordPress with this PR took 12.76s vs 12.84s on main (0% faster)

Generated by 🚫 Danger

Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! Looks good in principle.

What about the following example?

struct Outer {
  enum CodingKeys: String, CodingKey {
    case id
    
    struct S {}
  }
}

I think, the rule should trigger on S.

You need to run IntegrationTests locally and fix all linting issues it reports and update the configuration set reference file.

@@ -184,6 +184,25 @@ public extension EnumDeclSyntax {
return rawValueTypes.contains(identifier)
}
}

/// True if this enum is a `CodingKey`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// True if this enum is a `CodingKey`
/// True if this enum is a `CodingKey`. For that, it has to be named `CodingKeys` and must conform to the `CodingKey` protocol.

@@ -184,6 +184,25 @@ public extension EnumDeclSyntax {
return rawValueTypes.contains(identifier)
}
}

/// True if this enum is a `CodingKey`
var isCodingKey: Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum itself is not a coding key, but it defines the coding keys, doesn't it? So what about the name definesCodingKeys instead?

return false
}

guard self.name.text == "CodingKeys" else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
guard self.name.text == "CodingKeys" else {
guard name.text == "CodingKeys" else {

Can also be added to the previous guard.

Comment on lines +199 to +203
guard let identifier = element.type.as(IdentifierTypeSyntax.self)?.name.text else {
return false
}

return identifier == "CodingKey"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
guard let identifier = element.type.as(IdentifierTypeSyntax.self)?.name.text else {
return false
}
return identifier == "CodingKey"
element.type.as(IdentifierTypeSyntax.self)?.name.text == "CodingKey"

@@ -64,7 +64,9 @@ private extension NestingRule {
}

override func visit(_ node: EnumDeclSyntax) -> SyntaxVisitorContinueKind {
validate(forFunction: false, triggeringToken: node.enumKeyword)
if !(configuration.ignoreCodingKeys && node.isCodingKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if !(configuration.ignoreCodingKeys && node.isCodingKey) {
if !configuration.ignoreCodingKeys || !node.isCodingKey {

.init("""
struct Outer {
struct Inner {
enum Example: String, CodingKey {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
enum Example: String, CodingKey {
enum Example: String, CodingKey {

@@ -15,6 +15,8 @@ struct NestingConfiguration: RuleConfiguration {
private(set) var alwaysAllowOneTypeInFunctions = false
@ConfigurationElement(key: "ignore_typealiases_and_associatedtypes")
private(set) var ignoreTypealiasesAndAssociatedtypes = false
@ConfigurationElement(key: "ignore_codingkeys")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@ConfigurationElement(key: "ignore_codingkeys")
@ConfigurationElement(key: "ignore_coding_keys")

@@ -16,6 +16,10 @@
improvements done in the [SwiftSyntax](https://github.com/apple/swift-syntax)
library.

* Add `ignore_codingkeys` parameter to `nesting` rule.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain briefly what this option does.

Suggested change
* Add `ignore_codingkeys` parameter to `nesting` rule.
* Add `ignore_coding_keys` parameter to `nesting` rule.

@braker1nine
Copy link
Contributor Author

FYI I'm out of town until next weekend. I'll jump back into this once I'm back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Request: Ignore CodingKey enums in Nesting
3 participants