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 identifier_name check enum-element-with-associated-value error #2272

Closed
wants to merge 2 commits into from

Conversation

gnou
Copy link

@gnou gnou commented Jun 28, 2018

#2255 was the fix of #2231 , but in this(#2255) fix, @marcelofabri set a Swift lang version check, only check >4.1, which makes it ineffectual if you're using Xcode 10 & Swift 4.1, I removed that constraint in my pull request.

@marcelofabri
Copy link
Collaborator

@gnou Even if you are using Swift 4.1, it's actually reported as 4.2 by SwiftLint because that's the compiler version. Have you tried compiling master and running against your project?

@gnou
Copy link
Author

gnou commented Jun 28, 2018

@marcelofabri I did, this works for my project.

@SwiftLintBot
Copy link

SwiftLintBot commented Jun 28, 2018

12 Messages
📖 Linting Aerial with this PR took 0.41s vs 0.37s on master (10% slower)
📖 Linting Alamofire with this PR took 3.3s vs 3.12s on master (5% slower)
📖 Linting Firefox with this PR took 13.52s vs 11.71s on master (15% slower)
📖 Linting Kickstarter with this PR took 20.81s vs 17.23s on master (20% slower)
📖 Linting Moya with this PR took 2.19s vs 1.89s on master (15% slower)
📖 Linting Nimble with this PR took 1.84s vs 1.6s on master (15% slower)
📖 Linting Quick with this PR took 0.52s vs 0.5s on master (4% slower)
📖 Linting Realm with this PR took 3.79s vs 3.2s on master (18% slower)
📖 Linting SourceKitten with this PR took 1.18s vs 0.99s on master (19% slower)
📖 Linting Sourcery with this PR took 5.03s vs 4.5s on master (11% slower)
📖 Linting Swift with this PR took 33.46s vs 27.59s on master (21% slower)
📖 Linting WordPress with this PR took 15.87s vs 15.16s on master (4% slower)

Generated by 🚫 Danger

@codecov-io
Copy link

Codecov Report

Merging #2272 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2272      +/-   ##
==========================================
- Coverage   92.11%   92.11%   -0.01%     
==========================================
  Files         286      286              
  Lines       14533    14532       -1     
==========================================
- Hits        13387    13386       -1     
  Misses       1146     1146
Impacted Files Coverage Δ
.../SwiftLintFramework/Rules/IdentifierNameRule.swift 96.47% <ø> (-0.05%) ⬇️

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 1c8d5a4...66f89b4. Read the comment docs.

@gnou
Copy link
Author

gnou commented Jun 28, 2018

So here's what happened:
Hours ago, I brew install SwiftLint, and found it handled enum-case-with-associated-value wrong, then I found 2 pull requests regarding this exact issue(#2231), #2232 & #2255 , judge from the date of pull requests, I guess it's not in the released version, so I build from master branch, but the issue was still existed. After remove swift-lang-version-check, and increased version number to 0.26.1, build a new one, this time it works.
I'm still confused regarding how SwiftVersion is determined, because I tried to run testIdentifierName() in IdentifierNameRuleTests.swift by using Xcode 10, and it says SwiftVersion.current is 3, that makes no sense. I assume it's a bug in Xcode 10.
Bottom line, I don't think swift-lang-version-check is necessary is this case, as long as it's enumelement we're checking, we can safely check and remove () from it.

@marcelofabri
Copy link
Collaborator

I assume it's a bug in Xcode 10.

This was an issue with SourceKitten that should be fixed on master. Can you please double check?

I don't think swift-lang-version-check is necessary in this case, as long as it's enumelement we're checking, we can safely check and remove () from it.

I agree, but at the same time, I didn't want to penalize users on Swift < 4.2 with a more expensive operation. Also, since Swift 4.2 is still in beta, the behavior might change.

@marcelofabri
Copy link
Collaborator

Closing this for now.

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

4 participants