-
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
Add DuplicateImportsRule
#2004
Add DuplicateImportsRule
#2004
Conversation
Hi @sammy-SC. I don't know useful my suggestion would be for the implementation. But an easy way to find dups is using a |
Hello @ornithocoder I was considering that approach however I couldn't find a way to deal with nested modules.
and this should throw a warning on first line as that module has already been imported. Hope it makes sense and explains why I chose to go with comparing lines among themselves with two nested for loops. |
Codecov Report
@@ Coverage Diff @@
## master #2004 +/- ##
==========================================
+ Coverage 91.92% 91.97% +0.04%
==========================================
Files 307 309 +2
Lines 15333 15452 +119
==========================================
+ Hits 14095 14212 +117
- Misses 1238 1240 +2
Continue to review full report at Codecov.
|
@ornithocoder
I don't know how to reliably detect this. @jpsim @marcelofabri how would you go about detecting Also to me it doesn't look like valid use case of Thank you |
kind: .style, | ||
nonTriggeringExamples: [ | ||
"import A\nimport B\nimport C", | ||
"import A.B\nimport import A.C" |
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.
There's a double import
for A.C
.
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.
@ornithocoder thank you, I've fixed that.
Hi @sammy-SC. Regarding the {
"key.diagnostic_stage" : "source.diagnostic.stage.swift.parse",
"key.substructure" : [
],
"key.offset" : 0,
"key.length" : 67
} But "shows" on the syntax map. [
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 0,
"length" : 3
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.id",
"offset" : 4,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.attribute.builtin",
"offset" : 14,
"length" : 9
},
{
"type" : "source.lang.swift.syntaxtype.keyword",
"offset" : 24,
"length" : 6
},
{
"type" : "source.lang.swift.syntaxtype.identifier",
"offset" : 31,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 37,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.keyword",
"offset" : 47,
"length" : 6
},
{
"type" : "source.lang.swift.syntaxtype.identifier",
"offset" : 54,
"length" : 5
},
{
"type" : "source.lang.swift.syntaxtype.buildconfig.keyword",
"offset" : 60,
"length" : 6
}
] But honestly, I don't know what to do in this situation. Probably ignore duplicates if they're wrapped by flags? Specially if the |
Hello @ornithocoder I've done what you suggested, ignoring imports in conditional compilation directives. |
I'm curious on why https://github.com/apple/swift/blob/d01c9fa136bb35b801acb948ab2ee4f1d6cb3e9c/stdlib/public/Platform/msvcrt.swift#L14:1 triggered the warning 🤔 |
b1099b7
to
d864afa
Compare
@marcelofabri I'm sorry I must have missed that. |
Hmm, apparently this rule made linting way slower on oss-check:
I'm re-running it just to be sure it wasn't caused by a Circle CI instability. |
@marcelofabri are the new results sufficient or should I look into improving performance? |
@sammy-SC I think it's fine now. I'll try to review this in the following days. |
@marcelofabri could you please check this PR when you have time? I think it would be a useful addition to SwiftLint. |
@sammy-SC Sorry for the delay. I think we should make sure we're handling all the import forms, including the less-known import kind (https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/Declarations.html#//apple_ref/doc/uid/TP40014097-CH34-ID354). Or we can skip that for now and make the rule opt-in. |
import Foundation | ||
import SourceKittenFramework | ||
|
||
extension Line { |
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.
this extension should be private
(and I would move it to the end of this file)
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.
Done
identifier: "duplicate_imports", | ||
name: "Duplicate Imports", | ||
description: "Imports should be unique.", | ||
kind: .style, |
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.
.idiomatic
?
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.
Changed.
guard let kind = SyntaxKind(rawValue: token.type) else { | ||
return false | ||
} | ||
return kind == SyntaxKind.buildconfigKeyword |
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.
you can use just kind == .buildconfigKeyword
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.
Changed
@@ -5,6 +5,7 @@ | |||
// Created by Scott Hoyt on 12/28/15. | |||
// Copyright © 2015 Realm. All rights reserved. | |||
// | |||
|
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.
🤔
Did you manually add this line?
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'm sorry, I removed the line
c7e1c8c
to
f96068b
Compare
@marcelofabri Please when you have a time could you take another look at this? Thank you |
@marcelofabri could you please rerun the speed check? |
afd2bbb
to
761f75e
Compare
@marcelofabri I resolved merge conflicts. |
@sammy-SC I ran oss-check locally and it seems it's not too bad:
|
@marcelofabri please let me know if there is anything else you would like me to change. Any tips on improving the code are very welcome, even if it was to improve readability / clarify of the solution. If approved I would be more than happy to add autocorrection. |
OSSCheck highlighted a number of false positives. For example, import Realm
import Realm.Private |
@jpsim that's intentional, is that not correct? The idea was that if you have imports such as
would get highlighted as duplicate import. Is that not desired? If not, I'm happy to change it. |
However, I think this is fairly rare in practice that the false positive may be worth the ability to find duplicate Apple module imports, which as you point out are usually re-exported by parents. |
761f75e
to
6b4bb80
Compare
Thank you for being so patient on this one @sammy-SC! I finally had some time to update this, refactor it a little and came to a decision on keeping the rule more powerful but with a few rare false positives. |
@jpsim thanks for the help. I will check out the refactoring. It took a little over a year so not too bad :) |
implements #1881
Any recommendations for improvement are very welcome.
if you are happy with implementation, I will be happy to get started on autocorrection.