Recursive Configs #301

scottrhoyt · 2015-12-27T23:47:04Z

Closes #299

Ok, so here is my first stab at the recursive folder configurations. I think that using a recursive folder search method for the entire linting process would greatly optimize nested configurations, but I implemented this using the simplest algorithm with the current linting search method.

Basically, the configuration to use in linting a file is now a function of the file. To find this configuration, we recursively search backwards from directory of the file until we either find the current config file, reach the root of the lint search, or we find a configuration file to merge with. In this initial implementation, merge is simply overriding the config.

NOTE I primarily put the additional code in an extension to avoid exceeding the line limit for the Configuration type, though I suppose it has some encapsulation value to it.

Here are the things that I am still planning to add to this PR:

Config option to use nested configs (defaults to false for performance)
Refactor initializing Configuration from a yml to be more efficient (it's getting called a lot more)
Suppress spamming "Loading configuration from " messages intelligently

Let me know what you think so far and I'll continue adding commits to this PR.

jpsim · 2015-12-28T15:47:47Z

Source/SwiftLintFramework/Extensions/QueuedPrint.swift

-public func queuedPrintError(string: String) {
-    dispatch_async(outputQueue) {
-        fputs(string + "\n", stderr)
+public func queuedPrintError(string: String, silent: Bool = false) {


Instead of a silent parameter that just bypasses the function like this, I'd rather just not call the function in the first place. Could you please revert this change?

Sure, but because the new code results in a new configuration load for every file under a nested configuration, reverting this will result in dozens of "Loading configuration from " messages when using nested configs. I found this really obfuscates the logs. Another option would be to suppress the logging inside the init function, but that didn't seem right to me. My intention here was to centralize the logging logic in one place so that the logger could decide how to process. Because we wouldn't have to hunt down selective logging in methods, this could potentially ease the transition to more robust logging if needed. So let me know which of the three solutions you'd like:

Revert to original, which is the simplest but logs will be muddled with new load messages

Put the selective logging logic in the init function which requires no change to logging functions, but makes init more complicated and sets the precedent of distributing logging logic to non-logging objects.

Put the selective logging logic in the logger which solves the above problems but results in a degenerative trivial silent logging case

Put the selective logging logic in the init function which requires no change to logging functions, but makes init more complicated and sets the precedent of distributing logging logic to non-logging objects.

This is what I was trying to say. The logging in Configuration's initializer is already unfortunate but in fact, it should be relatively straightforward to move it to the swiftlint CLI app instead, which would address the need to silence some configuration loads without polluting the model object with logging code.

Ideally, all logs would be in swiftlint with none in SwiftLintFramework.

Ok, I will revert this and instead put the conditional logging inside of Configuration.init. Stay tuned! :)

This reverts commit d8c418e.

scottrhoyt · 2015-12-29T02:28:28Z

Logging is now suppressed in Configuration.init(...).

jpsim · 2015-12-29T18:24:14Z

Source/SwiftLintFramework/Models/Configuration.swift

-                queuedPrintError("Loading configuration from '\(path)'")
-                self.init(yaml: yamlContents)!
+            if let yamlConfig = Configuration.loadYaml(yamlContents) {
+                if !silent {


👍 much better IMO

jpsim · 2015-12-29T18:30:30Z

This is looking quite good, @scottrhoyt! Please don't let my many comments discourage you 😉.

It would be nice to have some tests for this. The rest of the project is pretty well tested IMO, so it'd be nice to keep that up here.

It might make sense to introduce a Fixtures/ directory in SwiftLintFrameworkTests/ since much of this functionality relies on directory structure. Otherwise, some straightforward unit tests could be sufficient.

…RuleParameter conform to Equatable. Wrote tests to verify. All done in support of testing recursive configuration file search.

…ssing test.

scottrhoyt · 2015-12-30T20:48:55Z

@jpsim No worries. I can definitely sympathize with being a test nut. I wrote some tests for for this in the best way I could come up with. All in all, writing the tests took longer than the code (I hate when that happens), but that was mostly because of the things I needed to support the tests. This is what I did:

Enabled code coverage generation for the project
Added the isEqual method to the Rule and ParameterizedRule protocols via extensions
Wrote conformance for RuleParameter and Configuration to Equatable
Wrote tests for all of the above
Wrote a test for Configuration.merge
Created a mock project structure in the Resources folder of the testing bundle with mock Swift files and mock *.yml's
Tested recursive configuration search with the mock project structure.

Let me know what you think.

jpsim · 2015-12-30T23:51:11Z

Source/SwiftLintFramework/Protocols/Rule.swift

+
+func == (lhs: [Rule], rhs: [Rule]) -> Bool {
+    if lhs.count == rhs.count {
+        return zip(lhs, rhs).map { $0.isEqualTo($1) }.reduce(true) { $0 && $1 }


I think you can avoid creating the intermediate closures here:

zip(lhs, rhs).map(Rule.isEqualTo).reduce(true, &&)

jpsim · 2015-12-30T23:51:55Z

Source/SwiftLintFrameworkTests/Resources/ProjectMock/Level1/Level2/.swiftlint.yml

+  - "the place where i commited many coding sins"
+line_length: 10000000000
+reporter: "json"
+use_nested_configs: true


missing trailing newline here

jpsim · 2015-12-31T01:10:40Z

This is looking really good, could you please add an entry to the CHANGELOG following these guidelines and add some basic documentation in the README?

scottrhoyt · 2015-12-31T18:41:09Z

@jpsim CHANGELOG and README updated. Let me know if you need anything else.

* realm/master: use SourceKitten 0.7.3 release [README] `excluded` overrides `included` only generate violations once for triggering examples in verifyRule() Support command comment modifiers [CHANGELOG] Fix entry [CHANGELOG] add entry Fix realm#295 Add failing test to ValidDocsRule.swift [CHANGELOG] Fix entry test violation locations [CHANGELOG] add entry Fix realm#294 # Conflicts: # CHANGELOG.md # README.md

scottrhoyt · 2015-12-31T18:47:36Z

I pulled master into my branch to resolve the conflicts in README and CHANGELOG

scottrhoyt · 2015-12-31T18:58:00Z

Carthage/Checkouts/SourceKitten

@@ -1 +1 @@
-Subproject commit 0ed186552d8f9f3e128ab382581102c55c2d3c73


@jpsim might want to make sure this is as intended? Seems to have been switched during merging master.

jpsim · 2016-01-03T00:13:03Z

Many many thanks for this work @scottrhoyt! I made a few tweaks and merged in #307.

scottrhoyt added 6 commits December 27, 2015 17:21

Added String extensions.

12a4316

Added recursive search for configurations.

8147a3e

Use new String extension.

c8fc563

Added use_nested_configs option for .swiftlint.yml (defaults to no).

cded5c9

Refactor Configuration initialization from yml for efficiency.

0bf09ef

Silently load nested configurations.

d8c418e

jpsim reviewed Dec 28, 2015
View reviewed changes

scottrhoyt mentioned this pull request Dec 28, 2015

Switch to dictionary format for parameterized rules #303

Closed

scottrhoyt added 2 commits December 28, 2015 17:19

Revert "Silently load nested configurations."

a38abbb

This reverts commit d8c418e.

Silence the logging inside of Configuration.init instead.

27ccc06

jpsim reviewed Dec 29, 2015
View reviewed changes

scottrhoyt added 8 commits December 29, 2015 12:59

Updated comments.

aa5dd2c

Enable test coverage generation.

6e336a5

Added isEqual for Rule and ParameterizedRule. Made Configuration and …

599f3f1

…RuleParameter conform to Equatable. Wrote tests to verify. All done in support of testing recursive configuration file search.

Moved == implementation for [Rule] to more appropriate spot. Wrote mi…

17cecab

…ssing test.

Extracted Configuration mocks to test class constants.

44cb7a5

Tested merge.

277d693

Added Nested Configuration testing.

a974317

Replace manual config mocks with file-driven.

87afad3

No longer needed RuleMocks at global scope.

f49003e

jpsim reviewed Dec 30, 2015
View reviewed changes

Cleaned up project mock *.yml's.

a523cf9

scottrhoyt added 2 commits December 31, 2015 10:43

Updated CHANGELOG

2ef4701

Update README and CHANGELOG

37b462d

scottrhoyt reviewed Dec 31, 2015
View reviewed changes

jpsim mentioned this pull request Jan 2, 2016

Recursive Configs #307

Merged

jpsim closed this Jan 3, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive Configs #301

Recursive Configs #301

scottrhoyt commented Dec 27, 2015

jpsim Dec 28, 2015

scottrhoyt Dec 28, 2015

jpsim Dec 28, 2015

scottrhoyt Dec 28, 2015

scottrhoyt commented Dec 29, 2015

jpsim Dec 29, 2015

jpsim commented Dec 29, 2015

scottrhoyt commented Dec 30, 2015

jpsim Dec 30, 2015

jpsim Dec 30, 2015

jpsim commented Dec 31, 2015

scottrhoyt commented Dec 31, 2015

scottrhoyt commented Dec 31, 2015

scottrhoyt Dec 31, 2015

jpsim commented Jan 3, 2016

		@@ -1 +1 @@
		Subproject commit 0ed186552d8f9f3e128ab382581102c55c2d3c73

Recursive Configs #301

Recursive Configs #301

Conversation

scottrhoyt commented Dec 27, 2015

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

scottrhoyt commented Dec 29, 2015

Choose a reason for hiding this comment

jpsim commented Dec 29, 2015

scottrhoyt commented Dec 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Dec 31, 2015

scottrhoyt commented Dec 31, 2015

scottrhoyt commented Dec 31, 2015

Choose a reason for hiding this comment

jpsim commented Jan 3, 2016