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

Autocorrect reports success even though nothing happens #466

Closed
tkohout opened this issue Feb 2, 2016 · 10 comments · Fixed by #499
Closed

Autocorrect reports success even though nothing happens #466

tkohout opened this issue Feb 2, 2016 · 10 comments · Fixed by #499
Assignees
Labels

Comments

@tkohout
Copy link

tkohout commented Feb 2, 2016

When I call swiftlint autocorrect

I get a report like this:

.
.
.
/Source/TabBarController.swift:51:25 Corrected Comma Spacing
/Source/TabBarController.swift:50:67 Corrected Comma Spacing
/Source/TabBarController.swift:49:101 Corrected Comma Spacing
Done correcting 30 files!

But no file is modified,

I noticed it happens with
Statement Position and Comma Spacing, but there might be more.

Other rules get corrected corectly. It would be nice to at least to not display the success message if the rule fails to correct the error. It's a bit confusing.

Thanks

@tkohout tkohout changed the title Autocorrect reports correcting even though nothing happens Autocorrect reports success even though nothing happens Feb 2, 2016
@jpsim jpsim added the bug label Feb 6, 2016
@jpsim
Copy link
Collaborator

jpsim commented Feb 6, 2016

@tkohout could you please share code that reproduces this issue? We have tests in place that ensure that the corrections are successfully applied, maybe they're incomplete? See https://github.com/realm/SwiftLint/blob/0.8.0/Source/SwiftLintFramework/Rules/CommaRule.swift#L32-L37

Other rules get corrected corectly. It would be nice to at least to not display the success message if the rule fails to correct the error. It's a bit confusing.

Obviously this is a bug. It's not SwiftLint's design to print that something was corrected when in fact it wasn't.

@jpsim
Copy link
Collaborator

jpsim commented Feb 6, 2016

I've reproduced this by running swiftlint autocorrect on SwiftLint's codebase itself:

Source/swiftlint/Commands/LintCommand.swift:39:17 Corrected Statement Position
Source/swiftlint/Extensions/Configuration+CommandLine.swift:100:9 Corrected Statement Position
Source/SwiftLintFramework/Extensions/Array+SwiftLint.swift:15:9 Corrected Statement Position
Source/SwiftLintFramework/Extensions/File+SwiftLint.swift:114:13 Corrected Statement Position
Source/SwiftLintFramework/Extensions/File+SwiftLint.swift:112:13 Corrected Statement Position
Source/SwiftLintFramework/Extensions/File+SwiftLint.swift:61:9 Corrected Statement Position
Source/SwiftLintFramework/Extensions/File+SwiftLint.swift:58:13 Corrected Statement Position
Source/SwiftLintFramework/Extensions/NSFileManager+SwiftLint.swift:31:9 Corrected Statement Position
Source/SwiftLintFramework/Extensions/NSFileManager+SwiftLint.swift:28:13 Corrected Statement Position
Source/SwiftLintFramework/Extensions/Yaml+SwiftLint.swift:20:9 Corrected Statement Position
Source/SwiftLintFramework/Models/Command.swift:74:9 Corrected Statement Position
Source/SwiftLintFramework/Models/Configuration.swift:166:9 Corrected Statement Position
Source/SwiftLintFramework/Models/Configuration.swift:90:9 Corrected Statement Position
Source/SwiftLintFramework/Models/Location.swift:47:9 Corrected Statement Position
Source/SwiftLintFramework/Models/Location.swift:36:9 Corrected Statement Position
Source/SwiftLintFramework/Models/MasterRuleList.swift:34:13 Corrected Statement Position
Source/SwiftLintFramework/Models/MasterRuleList.swift:30:17 Corrected Statement Position
Source/SwiftLintFramework/Models/YamlParser.swift:46:9 Corrected Statement Position
Source/SwiftLintFramework/Models/YamlParser.swift:36:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/ControlStatementRule.swift:94:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:38:14 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:31:15 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:37:14 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:30:15 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:39:14 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:32:15 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:40:14 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:33:15 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:36:14 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/LegacyConstructorRule.swift:29:15 Corrected Legacy Constructor
Source/SwiftLintFramework/Rules/NestingRule.swift:51:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/OpeningBraceRule.swift:141:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/RuleConfigs/NameConfig.swift:72:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/RuleConfigs/NameConfig.swift:69:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/RuleConfigs/SeverityLevelsConfig.swift:48:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/RuleConfigs/SeverityLevelsConfig.swift:44:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/RuleConfigs/SeverityLevelsConfig.swift:16:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/TrailingNewlineRule.swift:74:9 Corrected Statement Position
Source/SwiftLintFramework/Rules/TypeNameRule.swift:71:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/TypeNameRule.swift:66:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/VariableNameRule.swift:74:13 Corrected Statement Position
Source/SwiftLintFramework/Rules/VariableNameRule.swift:69:13 Corrected Statement Position
Source/SwiftLintFrameworkTests/CustomRulesTests.swift:41:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/ExtendedNSStringTests.swift:34:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/RuleConfigTests.swift:105:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/RuleConfigTests.swift:93:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/RuleConfigTests.swift:46:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/TestHelpers.swift:110:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/TestHelpers.swift:106:9 Corrected Statement Position
Source/SwiftLintFrameworkTests/TestHelpers.swift:46:5 Corrected Statement Position
Source/SwiftLintFrameworkTests/Yaml+SwiftLintTests.swift:25:9 Corrected Statement Position

There were no changes for any of the reported "Statement Position" corrections. I believe this is because a no-op correction was applied (string was overwritten to the same value). I think the right solution here is to fix the cases in which this happens.

The "Legacy Constructor" corrections were correctly being reported, but incorrectly being corrected since the "violations" were in string literals, which the rule should be resilient against.

I'm investigating all this now.

@tkohout
Copy link
Author

tkohout commented Feb 7, 2016

Awesome, thanks for quick response

@jpsim
Copy link
Collaborator

jpsim commented Feb 7, 2016

Happy to help. These issues should be fixed by #499 and #501. Although I still haven't been able to reproduce issues with the "Comma Spacing" rule that you reported. I'd still appreciate a repro case for that one.

@jpsim jpsim closed this as completed in #499 Feb 7, 2016
@aldrich
Copy link

aldrich commented Feb 15, 2016

Still happens in 0.9.0, saying it corrected comma spacing for a file, but not modifying it.

@jpsim
Copy link
Collaborator

jpsim commented Feb 15, 2016

@aldrich as I mentioned in my previous comment, we have yet to reproduce issues with "Comma Spacing" rule. Can you share some sample code that demonstrates the issue?

@norio-nomura
Copy link
Collaborator

@jpsim I found reproducing example:

let a = [1,1] // produces violation, but not triggers correcting.
let b = 1
f(1, b) // does not produce violation, but triggers correcting.

@jpsim
Copy link
Collaborator

jpsim commented Feb 16, 2016

Cool, thanks @norio-nomura! Let's reopen this one.

@jpsim jpsim reopened this Feb 16, 2016
@norio-nomura norio-nomura self-assigned this Feb 16, 2016
norio-nomura added a commit that referenced this issue Feb 16, 2016
Now corrections happen only on violations.
Because `assertCorrection(_:expected:)` expects one correction for each `corrections`,
and the rule added by 93e8e12 causes two corrections on previous logic.

Fix #466
norio-nomura added a commit that referenced this issue Feb 17, 2016
Now corrections happen only on violations.
Because `assertCorrection(_:expected:)` expects one correction for each `corrections`,
and the rule added by 93e8e12 causes two corrections on previous logic.

Fix #466
@fbarraganef
Copy link

This issues is happening to me in the version 0.25.0 with the legacy_contructo rule

static func strikePrice(value: String) -> NSAttributedString { let attributeString: NSMutableAttributedString = NSMutableAttributedString(string: value) attributeString.addAttribute(NSBaselineOffsetAttributeName, value: 0, range: NSMakeRange(0, attributeString.length)) attributeString.addAttribute(NSStrikethroughStyleAttributeName, value: 1, range: NSMakeRange(0, attributeString.length)) return attributeString }

I'm running swiftlint autocorrect and the NSMakeRange is not changed.

Thanks,
Francisco

@marcelofabri
Copy link
Collaborator

@fbarraganef please create a new issue following the issue template. Thanks!

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

Successfully merging a pull request may close this issue.

6 participants