Add remote, parent & child configuration features #3058

fredpi · 2020-01-28T11:40:13Z

This PR closes #1352 and #2729.

It's basically the same as #2824, but I squashed all commits to keep it more comprehensible.

Change Overview

Using multiple configurations is now documented extensively in the README
You can now specify child_config / parent_config configurations. This even works recursively, as long as there are no cycles and no ambiguities.
Nested configurations got a speed bump (via caching) and of course now ignore configs that were already used to build the primary config (using child_config / parent_config references)
You can pass multiple configurations via the command line that are interpreted as a hierarchy
You can specify a remote child_config / parent_config! This is just easy as dropping in a url that starts with http://or https:// where normally there would be the path to a local file. Remote configs get cached (in a manner that git doesn't see it). There are timeouts to loading remote configs, that can also be customized using remote_timeout / remote_timeout_if_cached
Rule Configurations now get merged properly (Previously, the rule configurations from the parent configuration were always used). This change results in the following behavior:
- When a child config doesn't configure a rule, yet the parent config does, the parent config will be used
- When a child config configures a rule, and the parent doesn't, the child config will be used
- When a child config configures a rule, and the parent does, too, the child config will be used
Warnings about duplicate and invalid identifiers now appear as actual warnings in Xcode
Internally, the whole Configuration clutter has been cleaned up in a way that I hope everyone is happy with. I strongly felt like the current merging implementation that relied on a rules array and a rulesMode didn't express the very foundation of what a configuration is: A mode that is applied to an array of all available rules with configurations, resulting in an array of rules that should be linted. To express this very relation, I created the new RulesWrapper type. While offering a resultingRules variable that is lazily computed, it still retains what caused these rules and therefore eases understanding of the merging algorithm.

Sidenote: Fixed Warnings in Moya

SwiftLintBot will probably complain that, with this new implementation, 3 warnings in Moya are now fixed. In fact, previously those have been false positives, as in the nested .swiftlint.yml covering these files there's the following configuration for the file_length rule:

file_length:
  warning: 1000

As the parent rule configuration was used for merging and there was no smart mechanism to decide which configuration to choose, the configuration originating from the nested configuration was ignored. With this PR, that's now fixed.

SwiftLintBot · 2020-01-28T12:02:12Z

	1 Warning
⚠️	Big PR

	12 Messages
📖	Linting Aerial with this PR took 1.97s vs 1.94s on master (1% slower)
📖	Linting Alamofire with this PR took 2.85s vs 2.83s on master (0% slower)
📖	Linting Firefox with this PR took 9.42s vs 9.42s on master (0% slower)
📖	Linting Kickstarter with this PR took 15.64s vs 15.69s on master (0% faster)
📖	Linting Moya with this PR took 1.46s vs 1.41s on master (3% slower)
📖	Linting Nimble with this PR took 1.34s vs 1.35s on master (0% faster)
📖	Linting Quick with this PR took 0.59s vs 0.63s on master (6% faster)
📖	Linting Realm with this PR took 4.41s vs 4.39s on master (0% slower)
📖	Linting SourceKitten with this PR took 1.08s vs 1.06s on master (1% slower)
📖	Linting Sourcery with this PR took 7.25s vs 7.33s on master (1% faster)
📖	Linting Swift with this PR took 11.31s vs 11.32s on master (0% faster)
📖	Linting WordPress with this PR took 17.54s vs 17.58s on master (0% faster)

Generated by 🚫 Danger

fredpi · 2020-01-28T14:57:23Z

@jpsim I added the missing tests (didn't come up with a proper idea how to test remote configs though), so this is now finally ready for review 🎉

fredpi · 2020-02-11T15:18:25Z

I just rebased to fix merge conflicts

fredpi · 2020-03-13T14:58:49Z

I just rebased to fix conflicts.

fredpi · 2020-04-15T12:29:05Z

I just rebased to fix conflicts.

mokagio · 2020-06-12T01:35:56Z

Is there anything I can do to help this get merged and shipped?

fredpi · 2020-06-17T21:16:59Z

@mokagio Thanks for asking and offering your help! 👍

From my side, this PR is ready and only needs a review. But as it changes a lot of things quite significantly, such a review

will take time
should preferably be done by someone with in-depth-knowledge about this project (like @jpsim or @marcelofabri)

But as this is a big PR and especially with the current situation, nobody can blame them for not having reviewed this yet or expect them to review this right now.

So, what you can do, is what you already did: Test this PR in real projects (and post your results in this thread), so that we can find bugs or other issues that may still exist. That's really helpful and gives confidence that this PR actually works in real environments! 🚀

Also, if you have the time, you may also do a review! I just think that @jpsim or @marcelofabri or another experienced SwiftLint contributor should approve this PR, but that doesn't say that other opinions aren't valuable!

fredpi · 2020-08-09T09:36:14Z

I just rebased to fix merge conflicts.

codecov-commenter · 2020-08-11T21:28:44Z

Codecov Report

Merging #3058 into master will decrease coverage by 0.37%.
The diff coverage is 74.89%.

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   90.58%   90.21%   -0.38%     
==========================================
  Files         416      423       +7     
  Lines       20406    21033     +627     
==========================================
+ Hits        18484    18974     +490     
- Misses       1922     2059     +137

Impacted Files	Coverage Δ
...rk/Extensions/Configuration+IndentationStyle.swift	`100.00% <ø> (ø)`
...urce/SwiftLintFramework/Helpers/Reachability.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/AnalyzeCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/AutoCorrectCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/LintCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/RulesCommand.swift	`0.00% <0.00%> (ø)`
...iftlint/Extensions/Configuration+CommandLine.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Helpers/CommonOptions.swift	`0.00% <ø> (ø)`
...ource/swiftlint/Helpers/LintOrAnalyzeCommand.swift	`0.00% <0.00%> (ø)`
Tests/SwiftLintFrameworkTests/RuleTests.swift	`82.66% <ø> (ø)`
... and 57 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 d9ce579...6a19709. Read the comment docs.

codecov-io · 2020-10-07T17:04:31Z

Codecov Report

Merging #3058 into master will decrease coverage by 0.31%.
The diff coverage is 74.89%.

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   90.48%   90.17%   -0.32%     
==========================================
  Files         417      424       +7     
  Lines       20508    21135     +627     
==========================================
+ Hits        18557    19058     +501     
- Misses       1951     2077     +126

Impacted Files	Coverage Δ
...rk/Extensions/Configuration+IndentationStyle.swift	`100.00% <ø> (ø)`
...urce/SwiftLintFramework/Helpers/Reachability.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/AnalyzeCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/AutoCorrectCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/LintCommand.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Commands/RulesCommand.swift	`0.00% <0.00%> (ø)`
...iftlint/Extensions/Configuration+CommandLine.swift	`0.00% <0.00%> (ø)`
Source/swiftlint/Helpers/CommonOptions.swift	`0.00% <ø> (ø)`
...ource/swiftlint/Helpers/LintOrAnalyzeCommand.swift	`0.00% <0.00%> (ø)`
Tests/SwiftLintFrameworkTests/RuleTests.swift	`82.66% <ø> (ø)`
... and 60 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 da408b5...92efddf. Read the comment docs.

fredpi · 2020-11-09T23:11:41Z

Quick update: I finished the rebase and implemented the review feedback, but noticed that I still have to take a closer look on the rootPath / configurationPath semantics again. I hope to find some time for this within the next days.

sethfri · 2020-11-11T00:13:56Z

@fredpi This is incredible! I'd love to look this over and give feedback if you'd like once you're done finishing up what you mentioned in your last comment.

That said, this has been outstanding for a long time, so I don't blame you if you just want to get it merged

fredpi · 2020-11-15T13:43:48Z

@sethfri While I didn't test everything myself yet, I don't see any major issues anymore, so I'd love it if you would try out the PR and report any issues you see! 👍 Also, it would be great if you could have a look at the new behavior specification for multiple configurations in the README and tell me whether you agree with these specifications.

Of course, before this will get merged, I will also test everything myself again.

fredpi · 2020-11-15T13:48:17Z

@mokagio It would be great if you could also report whether this PR is still working for you ;)

See realm/SwiftLint#3058 (comment)

mokagio · 2020-11-19T01:37:45Z

@fredpi hey there! You're awesome for relentlessly working on this ❤️

I updated our experiment repo to use ed50938bb955f224bf9146eb0b89e1368e4aca67. It builds and runs fine, both locally and on CI. 👍

The only thing I noticed is that it's not respecting the excluded setting 🤔 .

As you can see in this CI build we're getting violation warnings from files in the Pods/ folder, but that's [excluded in the remote config being used].(https://github.com/Automattic/swiftlint-config/blob/59fc2405960928f7fddb6a5e0fb77b6ce8d6f9c5/.swiftlint.yml#L2-L3)

Screenshot included because as I discovered GitHub Actions doesn't retain the logs indefinitely (for free repos, at least). The first couple of lines are from a file in Pods/ and unexpected, the last from one of the sources in the project and expected.

fredpi · 2020-11-20T00:14:07Z

@mokagio Thanks for the feedback! 😃

I could reconstruct and understand the bug with the excluded paths being ignored (+ write a test for it). With the latest commit (a37d4487f071463b619a6ff7367edcb50daeb5ca) the bug should be fixed now. Would it be possible for you to test the latest version again and confirm whether the issue is now gone?

See realm/SwiftLint#3058 (comment)

mokagio · 2020-11-20T01:32:07Z

@fredpi thank you for your quick work! I updated to a37d4487f071463b619a6ff7367edcb50daeb5ca and it all looks good no. See this CI run.

👍 🙌 👏

fredpi · 2020-11-21T00:02:11Z

I have added some more tests, tested the PR myself again and feel quite confident now that everything is working properly. So, if no new concerns get raised until then, I'm going to merge this on Sunday.

@sethfri If you still want to take a look at this and don't have the time to do it until Sunday, please tell me – I would then delay the merging.

Provides: - a runtime environment on ubuntu that does not include the Swift toolchain. - supportintg `docker build https://github.com/realm/SwiftLint.git` - add `libcurl4-openssl-dev` as build time dependency that introduced by #3058

mokagio · 2020-12-07T03:08:43Z

I'm so happy this is in! ❤️

fredpi mentioned this pull request Jan 28, 2020

Add child and parent config configuration features #2824

Closed

fredpi force-pushed the feature/remote-child-parent-configs branch 2 times, most recently from 0f4ef95 to 3dbff86 Compare January 30, 2020 09:39

fredpi force-pushed the feature/remote-child-parent-configs branch from 3dbff86 to 97b61d7 Compare February 11, 2020 15:17

fredpi force-pushed the feature/remote-child-parent-configs branch from 97b61d7 to e92efda Compare February 14, 2020 22:16

fredpi force-pushed the feature/remote-child-parent-configs branch from e92efda to 0f38492 Compare March 13, 2020 14:56

fredpi force-pushed the feature/remote-child-parent-configs branch 2 times, most recently from e141d25 to 12e1795 Compare April 7, 2020 20:12

fredpi force-pushed the feature/remote-child-parent-configs branch from 12e1795 to 707ccf3 Compare April 15, 2020 05:37

This was referenced Jun 9, 2020

Add SwiftLint wordpress-mobile/WordPress-iOS-Shared#261

Closed

Fix cache directory resolution when using remote config file #3231

Merged

fredpi force-pushed the feature/remote-child-parent-configs branch from 7ddb50d to d95c3ac Compare June 10, 2020 10:34

mokagio mentioned this pull request Jun 11, 2020

[Heads Up] SwiftLint may soon support remote configurations, which won't work out of the box here ashfurrow/danger-ruby-swiftlint#148

Open

mokagio mentioned this pull request Jun 12, 2020

SwiftLint remote configuration option houndci/hound#1801

Open

fredpi force-pushed the feature/remote-child-parent-configs branch from d95c3ac to 9eaafc7 Compare June 27, 2020 10:07

fredpi force-pushed the feature/remote-child-parent-configs branch from 9eaafc7 to de94775 Compare August 9, 2020 09:34

fredpi force-pushed the feature/remote-child-parent-configs branch from de94775 to e51a40b Compare August 11, 2020 21:00

fredpi force-pushed the feature/remote-child-parent-configs branch from e51a40b to 6a19709 Compare September 13, 2020 14:32

fredpi force-pushed the feature/remote-child-parent-configs branch from 6a19709 to 92efddf Compare October 7, 2020 16:39

fredpi requested a review from jpsim October 7, 2020 16:40

fredpi force-pushed the feature/remote-child-parent-configs branch 2 times, most recently from 00f4a59 to 8a7018d Compare November 9, 2020 23:08

fredpi mentioned this pull request Nov 10, 2020

Change in behavior of nested configs #3417

Closed

2 tasks

fredpi force-pushed the feature/remote-child-parent-configs branch from 8a7018d to 39e8d84 Compare November 14, 2020 19:51

fredpi force-pushed the feature/remote-child-parent-configs branch from abc9a61 to ed50938 Compare November 16, 2020 21:42

mokagio added a commit to wordpress-mobile/WordPress-iOS-Shared that referenced this pull request Nov 19, 2020

Update SwiftLint version

7f7aba4

See realm/SwiftLint#3058 (comment)

fredpi force-pushed the feature/remote-child-parent-configs branch from 3e8317b to a37d448 Compare November 20, 2020 00:11

mokagio added a commit to wordpress-mobile/WordPress-iOS-Shared that referenced this pull request Nov 20, 2020

Update SwiftLint

f9fc228

See realm/SwiftLint#3058 (comment)

fredpi force-pushed the feature/remote-child-parent-configs branch from 4d094b8 to 3d20e27 Compare November 20, 2020 21:49

Add remote, parent & child configuration features

4c5a3f0

fredpi force-pushed the feature/remote-child-parent-configs branch from 3d20e27 to 4c5a3f0 Compare November 20, 2020 22:09

fredpi added 4 commits November 21, 2020 00:05

Fix issues originating from new set semantics

495d742

Adjust file naming in child / parent config tests

715a8db

Fix test resources access on Linux

28cb43a

Drop unneeded import statement

26aae94

fredpi merged commit b8f5b6a into master Nov 22, 2020

fredpi deleted the feature/remote-child-parent-configs branch November 22, 2020 09:56

norio-nomura mentioned this pull request Dec 4, 2020

Add Dockerfile #3455

Merged

marcelofabri mentioned this pull request Dec 18, 2020

Invalid rule identifier with nested configs and custom rules on SwiftLint 0.42.0 #3472

Closed

2 tasks

fredpi mentioned this pull request Feb 25, 2021

Fix issue that prevented the inclusion of a configuration file from a parent folder #3491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add remote, parent & child configuration features #3058

Add remote, parent & child configuration features #3058

fredpi commented Jan 28, 2020 •

edited

Loading

SwiftLintBot commented Jan 28, 2020 •

edited

Loading

fredpi commented Jan 28, 2020

fredpi commented Feb 11, 2020

fredpi commented Mar 13, 2020

fredpi commented Apr 15, 2020

mokagio commented Jun 12, 2020

fredpi commented Jun 17, 2020

fredpi commented Aug 9, 2020

codecov-commenter commented Aug 11, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

fredpi commented Nov 9, 2020

sethfri commented Nov 11, 2020

fredpi commented Nov 15, 2020

fredpi commented Nov 15, 2020

mokagio commented Nov 19, 2020 •

edited

Loading

fredpi commented Nov 20, 2020

mokagio commented Nov 20, 2020 •

edited

Loading

fredpi commented Nov 21, 2020

mokagio commented Dec 7, 2020

Add remote, parent & child configuration features #3058

Add remote, parent & child configuration features #3058

Conversation

fredpi commented Jan 28, 2020 • edited Loading

Change Overview

Sidenote: Fixed Warnings in Moya

SwiftLintBot commented Jan 28, 2020 • edited Loading

fredpi commented Jan 28, 2020

fredpi commented Feb 11, 2020

fredpi commented Mar 13, 2020

fredpi commented Apr 15, 2020

mokagio commented Jun 12, 2020

fredpi commented Jun 17, 2020

fredpi commented Aug 9, 2020

codecov-commenter commented Aug 11, 2020 • edited Loading

Codecov Report

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

fredpi commented Nov 9, 2020

sethfri commented Nov 11, 2020

fredpi commented Nov 15, 2020

fredpi commented Nov 15, 2020

mokagio commented Nov 19, 2020 • edited Loading

fredpi commented Nov 20, 2020

mokagio commented Nov 20, 2020 • edited Loading

fredpi commented Nov 21, 2020

mokagio commented Dec 7, 2020

fredpi commented Jan 28, 2020 •

edited

Loading

SwiftLintBot commented Jan 28, 2020 •

edited

Loading

codecov-commenter commented Aug 11, 2020 •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

mokagio commented Nov 19, 2020 •

edited

Loading

mokagio commented Nov 20, 2020 •

edited

Loading