Reenable cache #1516

jpsim · 2017-05-15T23:53:40Z

Continued from #1311. Addresses #1184.

jpsim · 2017-05-15T23:54:19Z

I'd love if you could review this too @victorpimentel.

SwiftLintBot · 2017-05-16T00:09:50Z

	12 Messages
📖	Linting WordPress with this PR took 9.39s vs 9.44s on master (0% faster)
📖	Linting Alamofire with this PR took 2.42s vs 2.24s on master (8% slower)
📖	Linting Firefox with this PR took 9.82s vs 9.4s on master (4% slower)
📖	Linting Kickstarter with this PR took 12.86s vs 12.58s on master (2% slower)
📖	Linting Moya with this PR took 0.32s vs 0.31s on master (3% slower)
📖	Linting Nimble with this PR took 1.32s vs 1.27s on master (3% slower)
📖	Linting Aerial with this PR took 0.38s vs 0.33s on master (15% slower)
📖	Linting Realm with this PR took 2.01s vs 1.94s on master (3% slower)
📖	Linting SourceKitten with this PR took 0.88s vs 0.84s on master (4% slower)
📖	Linting Sourcery with this PR took 2.11s vs 2.02s on master (4% slower)
📖	Linting Swift with this PR took 9.08s vs 9.0s on master (0% slower)
📖	Linting Quick with this PR took 0.44s vs 0.43s on master (2% slower)

Generated by 🚫 danger

codecov-io · 2017-05-16T00:21:46Z

Codecov Report

Merging #1516 into master will decrease coverage by 0.29%.
The diff coverage is 61.58%.

@@            Coverage Diff            @@
##           master    #1516     +/-   ##
=========================================
- Coverage   82.78%   82.49%   -0.3%     
=========================================
  Files         187      187             
  Lines        9342     9432     +90     
=========================================
+ Hits         7734     7781     +47     
- Misses       1608     1651     +43

Impacted Files	Coverage Δ
Source/swiftlint/Commands/LintCommand.swift	`0% <0%> (ø)`	⬆️
...swiftlint/Extensions/LinterCache+CommandLine.swift	`0% <0%> (ø)`	⬆️
...Framework/Extensions/NSFileManager+SwiftLint.swift	`70.58% <0%> (-9.42%)`	⬇️
.../Rules/RuleConfigurations/RegexConfiguration.swift	`68.62% <0%> (-25.97%)`	⬇️
...iftLintFramework/Protocols/RuleConfiguration.swift	`50% <0%> (-50%)`	⬇️
...s/SwiftLintFrameworkTests/ConfigurationTests.swift	`81.73% <0%> (-0.8%)`	⬇️
Source/SwiftLintFramework/Protocols/Rule.swift	`77.77% <0%> (-9.73%)`	⬇️
Source/SwiftLintFramework/Rules/CustomRules.swift	`90.56% <0%> (-7.4%)`	⬇️
Source/SwiftLintFramework/Models/Linter.swift	`71.95% <66.66%> (-0.15%)`	⬇️
...urce/SwiftLintFramework/Models/Configuration.swift	`80.57% <7.14%> (-4.64%)`	⬇️
... and 2 more

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 bee0784...da7ef33. Read the comment docs.

victorpimentel

LGTM 👍

victorpimentel · 2017-05-16T00:17:15Z

Source/SwiftLintFramework/Models/LinterCache.swift

        lock.lock()
-        var filesCache = (cache["files"] as? [String: Any]) ?? [:]
+        var filesCache = (cache[LinterCache.Key.files.rawValue] as? [String: Any]) ?? [:]


Very small code style question: is there any reason to use LinterCache.Key in some lines and just Key in others? It's not important, but I'm reviewing this from my phone so I'm not 100% sure if they refer to another type.

I (inconsistently) omitted LinterCache within its body since it could be inferred. This one slipped through. Fixed in 43e4ed6.

marcelofabri · 2017-05-16T14:04:00Z

CHANGELOG.md

 ##### Enhancements

+* Cache linter results for files unmodified since the previous linter run.
+  [Victor Pimentel](https://github.com/victorpimentel)
+  [#1184](https://github.com/realm/SwiftLint/issues/1184)


missing two trailing spaces on all entries 😅

new computer accidentally has whitespace = fix in its .gitconfig so it automatically trimmed trailing whitespace when rebasing. Promptly removed that.

marcelofabri · 2017-05-16T14:11:22Z

Source/SwiftLintFramework/Models/Configuration.swift

+    public var cacheDescription: String {
+        let cacheRulesDescriptions: [String: Any] = rules.reduce([:]) { accu, element in
+            var accu = accu
+            accu[type(of: element).description.identifier] = element.configurationDescription


the configurationDescription of ConditionalReturnsOnNewline is "N/A" for some reason. We probably should change that as well.

fixed in #1523.

marcelofabri · 2017-05-16T14:19:41Z

Source/SwiftLintFramework/Models/Configuration.swift

+            "root": rootPath ?? FileManager.default.currentDirectoryPath,
+            "rules": cacheRulesDescriptions
+        ]
+        if let prettyJSONData = try? JSONSerialization.data(withJSONObject: dict),


shouldn't this pass options to actually be a pretty? 😆

doesn't need to be pretty actually, since I'm just using this for serialization. fixed variable name.

marcelofabri · 2017-05-16T14:24:30Z

Also, does this handle nested configurations? (It doesn't look like it does)

…files with a weak algorithm.

applying minor edits. Remove `configurationDescription` from breaking changes in changelog since additions aren't breaking changes.

rather than use hardcoded, duplicate string literals

rather than just when using one of many code paths in one of many initializers

to provide more complete descriptions for cache invalidation purposes.

by respecting the value of ignoreCache rather than always passing 'true'

jpsim · 2017-05-16T19:13:05Z

I've updated/rebased this after #1522, #1523 & #1524 were merged. I also updated it to consider custom rules in cache invalidation.

In manual testing, I've confirmed that:

An individual file cache is invalidated when:

The file is modified, either by changing its contents or running touch file.swift

Entire cache is invalidated when:

.swiftlint.yml custom rules are added/removed/modified
disabled_rules, enabled_rules, opt_in_rules, whitelist_rules are added/removed/changed
a rule's configuration is changed in .swiftlint.yml

Cache is not invalidated when:

contents of .swiftlint.yml is reordered
comments or whitespace are added/removed in .swiftlint.yml
cache_path, excluded, included, reporter, swiftlint_version, use_nested_configs or warning_threshold are added/removed/changed

The only thing I can see doesn't work is nested configurations. I haven't really looked into what it would take to support that either. I suspect it wouldn't be too hard though.

marcelofabri · 2017-05-16T19:51:26Z

Source/SwiftLintFramework/Rules/RuleConfigurations/RegexConfiguration.swift

+        dict["message"] = message
+        dict["regex"] = regex.pattern
+        if let included = included?.pattern {
+            dict["included"] = included


doesn't dict["included"] = included?.pattern have the same effect? (this applies for all optional variables)

that would produce ["included": nil] rather than [:]

It doesn't 😅

var dict = [String: String]() dict["foo"] = nil dict["bar"] = "bar" let data = try! JSONSerialization.data(withJSONObject: dict) print(String(data: data, encoding: .utf8)!) // {"bar":"bar"}

👍 thanks

marcelofabri · 2017-05-16T19:52:49Z

The only thing I can see doesn't work is nested configurations. I haven't really looked into what it would take to support that either. I suspect it wouldn't be too hard though.

Do you want to merge this without supporting nested configurations? If so, I'd suggest to disable caching by default until we do support them.

jpsim · 2017-05-16T21:30:02Z

WIP support for nested configurations: jp-reenable-cache...jp-wip-cache-support-nested-configs

I have some cleaning up to do and tests to write, but it shows that it can be done fairly easily.

jpsim · 2017-05-16T21:30:27Z

I'd really rather not ship this (again) with known issues or limitations.

samskiter · 2017-05-17T10:04:16Z

Thanks so much for this @jpsim 👍

jpsim · 2017-05-18T22:04:20Z

Superseded by #1530.

jpsim requested a review from marcelofabri May 15, 2017 23:54

jpsim force-pushed the jp-reenable-cache branch from 3a1cc1c to d836b9f Compare May 15, 2017 23:55

victorpimentel approved these changes May 16, 2017

View reviewed changes

marcelofabri requested changes May 16, 2017

View reviewed changes

victorpimentel and others added 8 commits May 16, 2017 11:56

Change LinterCache to use file modification dates instead of hashing …

ab3b57b

…files with a weak algorithm.

move changelog entries to appropriate section

48834fb

applying minor edits. Remove `configurationDescription` from breaking changes in changelog since additions aren't breaking changes.

simplify FileManager.modificationDate(forFileAtPath:) impl

63c6b60

simplify Configuration.swift's usage of its default file manager

b50e0ce

add LinterCache.Key enum

286c840

rather than use hardcoded, duplicate string literals

derive Configuration.cacheDescription in all cases

b4854f0

rather than just when using one of many code paths in one of many initializers

add CacheDescriptionProvider protocol

07861f2

to provide more complete descriptions for cache invalidation purposes.

re-enable cache when running lint command

41509fe

by respecting the value of ignoreCache rather than always passing 'true'

jpsim force-pushed the jp-reenable-cache branch from 43e4ed6 to 41509fe Compare May 16, 2017 18:56

marcelofabri reviewed May 16, 2017

View reviewed changes

simplify RegexConfiguration.cacheDescription

da7ef33

marcelofabri approved these changes May 16, 2017

View reviewed changes

jpsim mentioned this pull request May 18, 2017

Reenable cache #1530

Merged

jpsim closed this May 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reenable cache #1516

Reenable cache #1516

jpsim commented May 15, 2017

jpsim commented May 15, 2017

SwiftLintBot commented May 16, 2017 •

edited

Loading

codecov-io commented May 16, 2017 •

edited

Loading

victorpimentel left a comment

victorpimentel May 16, 2017

jpsim May 16, 2017

marcelofabri May 16, 2017

jpsim May 16, 2017

marcelofabri May 16, 2017

jpsim May 16, 2017

marcelofabri May 16, 2017

jpsim May 16, 2017

marcelofabri commented May 16, 2017

jpsim commented May 16, 2017

marcelofabri May 16, 2017 •

edited

Loading

jpsim May 16, 2017

marcelofabri May 16, 2017 •

edited

Loading

jpsim May 16, 2017

marcelofabri commented May 16, 2017

jpsim commented May 16, 2017

jpsim commented May 16, 2017

samskiter commented May 17, 2017

jpsim commented May 18, 2017

Reenable cache #1516

Reenable cache #1516

Conversation

jpsim commented May 15, 2017

jpsim commented May 15, 2017

SwiftLintBot commented May 16, 2017 • edited Loading

codecov-io commented May 16, 2017 • edited Loading

Codecov Report

victorpimentel left a 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

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

marcelofabri commented May 16, 2017

jpsim commented May 16, 2017

marcelofabri May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented May 16, 2017

jpsim commented May 16, 2017

jpsim commented May 16, 2017

samskiter commented May 17, 2017

jpsim commented May 18, 2017

SwiftLintBot commented May 16, 2017 •

edited

Loading

codecov-io commented May 16, 2017 •

edited

Loading

marcelofabri May 16, 2017 •

edited

Loading

marcelofabri May 16, 2017 •

edited

Loading