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

Fix #2231 #2232

Closed
wants to merge 6 commits into from
Closed

Fix #2231 #2232

wants to merge 6 commits into from

Conversation

Coder-256
Copy link

SwiftLint fails with an error in enums with associated types in Xcode 10 due to the identifier_name rule. This fixes that error.

@SwiftLintBot
Copy link

SwiftLintBot commented Jun 5, 2018

12 Messages
📖 Linting Aerial with this PR took 0.39s vs 0.36s on master (8% slower)
📖 Linting Alamofire with this PR took 3.36s vs 3.08s on master (9% slower)
📖 Linting Firefox with this PR took 12.87s vs 11.33s on master (13% slower)
📖 Linting Kickstarter with this PR took 20.0s vs 16.41s on master (21% slower)
📖 Linting Moya with this PR took 1.99s vs 1.82s on master (9% slower)
📖 Linting Nimble with this PR took 1.77s vs 1.47s on master (20% slower)
📖 Linting Quick with this PR took 0.51s vs 0.49s on master (4% slower)
📖 Linting Realm with this PR took 3.74s vs 3.12s on master (19% slower)
📖 Linting SourceKitten with this PR took 1.07s vs 0.97s on master (10% slower)
📖 Linting Sourcery with this PR took 4.86s vs 4.31s on master (12% slower)
📖 Linting Swift with this PR took 30.25s vs 27.46s on master (10% slower)
📖 Linting WordPress with this PR took 15.23s vs 14.26s on master (6% slower)

Generated by 🚫 Danger

@@ -35,7 +35,7 @@ public struct IdentifierNameRule: ASTRule, ConfigurationProviderRule {
return []
}

let isFunction = SwiftDeclarationKind.functionKinds.contains(kind)
let isFunction = SwiftDeclarationKind.functionKinds.contains(kind) || kind == .enumelement
Copy link

Choose a reason for hiding this comment

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

Maybe for better meaning you could separate in two variables. Like:

let isFunction = SwiftDeclarationKind.functionKinds.contains(kind)
let isEnumelement = kind == .enumelement

Or jus update the isFunction to something like isFunctionOrEnumelement.

@marcelofabri
Copy link
Collaborator

We shouldn't change the rule behavior (i.e. oss-check shouldn't report any changes). The problem is that key.name for an enum element now contains the parameters labels. Apparently, key.nameoffset and key.namelength only contain the name range (without parameters), so we should use that instead.

@Coder-256
Copy link
Author

@marcelofabri I'm having some trouble testing locally (many sourcekit errors, which it looks like they're already working on in SourceKitten). Let's see if these changes pass tests.

@codecov-io
Copy link

Codecov Report

Merging #2232 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2232      +/-   ##
==========================================
+ Coverage   92.03%   92.04%   +<.01%     
==========================================
  Files         283      283              
  Lines       14246    14253       +7     
==========================================
+ Hits        13112    13119       +7     
  Misses       1134     1134
Impacted Files Coverage Δ
.../SwiftLintFramework/Rules/IdentifierNameRule.swift 98.83% <100%> (+0.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c0e7c8...0c6442e. Read the comment docs.

@dominik-hadl
Copy link

Can we get this merged in soon? :)

@marcelofabri
Copy link
Collaborator

marcelofabri commented Jun 10, 2018

I've tested this and it looks like SourceKit reports incorrect values for key.nameoffset and key.namelength is the enum case contains backticks (e.g. `private`).

We should fill an issue in bugs.swift.org and meanwhile we can get the substring from key.name up to the first (.

To avoid any potential issues, I'd only do that if kind == .enumcase && SwiftVersion.current > .fourDotOne.

@@ -85,6 +85,10 @@

#### Bug Fixes

* Fix identifier name false positive for enum cases with associated values in Xcode 10.
Copy link
Collaborator

Choose a reason for hiding this comment

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

requires two trailing spaces and lines should be limited to 80 chars as described in CONTRIBUTING.md: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

@krodak
Copy link
Contributor

krodak commented Jun 20, 2018

Any update on this?

@marcelofabri
Copy link
Collaborator

Closing this in favor of #2255

@Coder-256
Copy link
Author

Thank you @marcelofabri!

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.

None yet

7 participants