Added disable rule test to autocorrect #766

masters3d · 2016-08-22T05:09:51Z

codecov-io · 2016-08-22T05:20:16Z

Current coverage is 88.80% (diff: 87.50%)

Merging #766 into master will increase coverage by 0.40%

@@             master       #766   diff @@
==========================================
  Files            81         81          
  Lines          2767       2768     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2446       2458    +12   
+ Misses          321        310    -11   
  Partials          0          0

Powered by Codecov. Last update f0573e7...48a085c

jpsim · 2016-08-22T20:17:25Z

Source/SwiftLintFramework/Rules/ColonRule.swift


        let regularExpression = regex(pattern)
        let description = self.dynamicType.description
        var corrections = [Correction]()
        var contents = file.contents
        for range in matches.reverse() {
+            let location = Location(file: file, characterOffset: range.location)
+            let region = fileregions.filter {$0.contains(location)}.first
+            if region?.isRuleDisabled(self) == true {


Could be:

if region?.isRuleDisabled(self) == false { contents = regularExpression.stringByReplacingMatchesInString(contents, options: [], range: range, withTemplate: "$1: $2") corrections.append(Correction(ruleDescription: description, location: location)) }

region?.isRuleDisabled(self) returns false when region is nil

if region?.isRuleDisabled(self) == true { // returns false when region is nil continue } else {...}

plus I was trying to stay away from double negatives ;)
https://twitter.com/NachoSoto/status/762358223122927616

Good call. This then?

if let region = fileregions.filter {$0.contains(location)}.first where !region.isRuleDisabled(self) { contents = regularExpression.stringByReplacingMatchesInString(contents, options: [], range: range, withTemplate: "$1: $2") corrections.append(Correction(ruleDescription: description, location: location)) }

tests fail. when region is nil it doesn't fire but we need it to fire sometimes when there are no regions.

Alright, keep it as-is then

jpsim · 2016-08-22T20:18:38Z

Rather than duplicate this code in a bunch of different rule implementations, this should be made generic.

masters3d · 2016-08-22T20:35:58Z

Yeah that was my initial implementation but it wasn't granular .
#724 (comment)

Do you think we should go that route? it would be nice to have a default implementation that the correctable protocol would have.

jpsim · 2016-08-22T20:43:20Z

Do you think we should go that route? it would be nice to have a default implementation that the correctable protocol would have.

Yes, we should reuse the code for this as much as possible.

jpsim · 2016-08-23T18:45:46Z

This needs a rebase.

masters3d · 2016-08-23T19:29:02Z

This is ready for now. I'll refactor all the correctable rules to pull this logic into the protocol in another PR.

jpsim · 2016-08-23T19:42:06Z

@masters3d I don't see how it's important to land this just for a subset of rules, only to do it more generically later. Can we just do it right the first time?

masters3d · 2016-08-23T20:09:40Z

I guess the important part is the added new test that makes sure new correctable rules do not omit disable regions checks. The rules I changed were the ones failing that test. I didn't touch other correctable rules that passed the test.

I imagine if we provide a general facility for this checking that we would want to refactor all the correctable rules.
That's the part I think belongs to another PR since I can see it being quite big.

jpsim · 2016-08-23T20:30:34Z

I'd rather do this be done in a single swoop, personally.

masters3d · 2016-08-24T05:33:25Z

Sounds good. Currently all Rules check for disabled regions and correctable rules have similar code twice. I am thinking this logic should probably be moved up to the Rule protocol.
The only tricky part is that the repeated code happens in a for-loop for most rules.

public func validateFile(file: File) -> [StyleViolation] {

        for (eachLastLine, eachSectionCount) in linesSections {
            // Skips violation for areas where the rule is disabled
            let region = file.regions().filter {
                $0.contains(Location(file: file.path, line: eachLastLine.index, character: 0))
            }.first
            if region?.isRuleDisabled(self) == true {
                continue
            }
....

public func correctFile(file: File) -> [Correction] {

      for currentLine in file.lines {
            // Doesnt correct lines where rule is disabled
            let region = fileRegions.filter {
                $0.contains(Location(file: file.path, line: currentLine.index, character: 0))
            }.first
            if region?.isRuleDisabled(self) == true {
                correctedLines.append(currentLine.content)
                continue
            }
....

masters3d · 2016-08-29T18:01:03Z

This should be ready now. I was complicating the issue. I found a method on File that does what I needed.

jpsim · 2016-08-29T21:30:11Z

CHANGELOG.md

@@ -12,6 +12,10 @@

 * Adds `allow_private_set` configuration for the `private_outlet` rule.  
  [Rohan Dhaimade](https://github.com/HaloZero)
+
+* Fix locally disabled comma rule autocorrect.  


I don't think this message is accurate anymore?

jpsim · 2016-08-29T22:31:21Z

CHANGELOG.md

@@ -12,6 +12,10 @@

 * Adds `allow_private_set` configuration for the `private_outlet` rule.  
  [Rohan Dhaimade](https://github.com/HaloZero)
+
+* Added test to correctable rules to check `swiftlint-disable` before auto-correcting file.  


the command is swiftlint:disable

Also, I'd call this a bug fix and the fact that you added tests is an implementation detail. The entry should read something like this IMO:

Correctable rules no longer apply corrections if the rule is locally disabled.

sounds good. I think I've now mastered the git command line :)

jpsim · 2016-08-30T17:19:15Z

Source/SwiftLintFramework/Rules/LegacyCGGeometryFunctionsRule.swift

                .filter { $0.1.first == .Identifier }
                .map { ($0.0, pattern, template) }
        }).flatten().sort { $0.0.location > $1.0.location } // reversed

+        if matches.isEmpty { return []}


Closure spacing 😉 #770

jpsim · 2016-08-30T17:21:45Z

Thanks for your continued work on this @masters3d! Nearly there.

norio-nomura · 2016-10-24T11:19:57Z

Looks good to me.
Are there any required changes that block this merging?

masters3d · 2016-10-28T22:39:03Z

none that I am aware

norio-nomura · 2016-10-29T05:09:56Z

@masters3d Thanks for doing this! 🙏

jpsim · 2016-11-25T17:51:57Z

🎉

jpsim reviewed Aug 22, 2016
View reviewed changes

masters3d force-pushed the no-autocorrect-disabled-rules branch 2 times, most recently from c93689c to 46a6b09 Compare August 22, 2016 21:54

masters3d force-pushed the no-autocorrect-disabled-rules branch 3 times, most recently from 46a6b09 to 2c05b98 Compare August 23, 2016 19:14

masters3d force-pushed the no-autocorrect-disabled-rules branch 4 times, most recently from bbb50e5 to 57fdce4 Compare August 29, 2016 04:31

jpsim reviewed Aug 29, 2016
View reviewed changes

masters3d force-pushed the no-autocorrect-disabled-rules branch from 57fdce4 to e58c0b9 Compare August 29, 2016 22:26

jpsim reviewed Aug 29, 2016
View reviewed changes

masters3d force-pushed the no-autocorrect-disabled-rules branch 3 times, most recently from faa8652 to a080617 Compare August 30, 2016 00:35

masters3d force-pushed the no-autocorrect-disabled-rules branch from a080617 to 096951a Compare August 30, 2016 00:37

jpsim reviewed Aug 30, 2016
View reviewed changes

masters3d force-pushed the no-autocorrect-disabled-rules branch from 08c5397 to ed52ab3 Compare August 30, 2016 22:39

Added disable rule test to autocorrect

48a085c

masters3d force-pushed the no-autocorrect-disabled-rules branch from ed52ab3 to 48a085c Compare August 30, 2016 22:42

norio-nomura merged commit 21e3439 into realm:master Oct 29, 2016

masters3d deleted the no-autocorrect-disabled-rules branch October 29, 2016 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added disable rule test to autocorrect #766

Added disable rule test to autocorrect #766

masters3d commented Aug 22, 2016 •

edited

Loading

codecov-io commented Aug 22, 2016 •

edited

Loading

jpsim Aug 22, 2016

masters3d Aug 22, 2016

jpsim Aug 22, 2016

masters3d Aug 22, 2016

jpsim Aug 22, 2016

jpsim commented Aug 22, 2016

masters3d commented Aug 22, 2016

jpsim commented Aug 22, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 23, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 23, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 24, 2016

masters3d commented Aug 29, 2016

jpsim Aug 29, 2016

jpsim Aug 29, 2016

jpsim Aug 29, 2016

masters3d Aug 30, 2016

jpsim Aug 30, 2016

jpsim commented Aug 30, 2016

norio-nomura commented Oct 24, 2016

masters3d commented Oct 28, 2016

norio-nomura commented Oct 29, 2016

jpsim commented Nov 25, 2016

Added disable rule test to autocorrect #766

Added disable rule test to autocorrect #766

Conversation

masters3d commented Aug 22, 2016 • edited Loading

codecov-io commented Aug 22, 2016 • edited Loading

Current coverage is 88.80% (diff: 87.50%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Aug 22, 2016

masters3d commented Aug 22, 2016

jpsim commented Aug 22, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 23, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 23, 2016

jpsim commented Aug 23, 2016

masters3d commented Aug 24, 2016

masters3d commented Aug 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Aug 30, 2016

norio-nomura commented Oct 24, 2016

masters3d commented Oct 28, 2016

norio-nomura commented Oct 29, 2016

jpsim commented Nov 25, 2016

masters3d commented Aug 22, 2016 •

edited

Loading

codecov-io commented Aug 22, 2016 •

edited

Loading