Add support for merging nested configuration with parent configurations #1674
Conversation
stephanecopin
force-pushed
the
nested-configurations
branch
from
52730ce to
d2e6ae2
Compare
stephanecopin
force-pushed
the
nested-configurations
branch
from
d2e6ae2 to
26be12b
Compare
stephanecopin
changed the title
|
Thanks for starting work on this! It has been on my to‐do list for months now, but I’ve been to busy with other things. I’m excited to see it. |
|
Same here! Note that we're having some major issues with OSSCheck at the moment, which I'm debugging in #1675, so there's no need to pay attention to its (wildly inaccurate) report. |
Codecov Report
@@ Coverage Diff @@
## master #1674 +/- ##
==========================================
+ Coverage 87.35% 87.49% +0.14%
==========================================
Files 201 201
Lines 10000 10102 +102
==========================================
+ Hits 8735 8839 +104
+ Misses 1265 1263 -2
Continue to review full report at Codecov.
|
|
I should've looked at the OSSCheck report more closely, as it looks like it was already accurate. The only OSS repo which reports newly added violations is Moya, which uses nested SwiftLint configurations (one at the top level and one under Tests), which would explain why this PR suddenly changes the violations reported under |
|
I've been playing around with this branch, splitting out different parts of Configuration.swift into dedicated extensions. You can see my edits in the jp-nested-configurations branch, or more usefully as a comparison with this PR's branch: stephanecopin/SwiftLint@nested-configurations...realm:jp-nested-configurations @stephanecopin I'd love to hear your rough thoughts on that. I haven't had the time to clean it up much, but if you like some of the directions, we can iterate on it. |
|
@jpsim I very much like that, that makes the whole thing less cluttered. I think this is the right direction 👍 |
|
Continued in #1687. |
|
The README seems to be outdated about Nested Configurations. It looks like this PR says we CAN use |
That's right! Since August 2017, any nested configuration's return Configuration(
rulesMode: configuration.rulesMode, // Use the rulesMode used to build the merged configuration
included: configuration.included, // Always use the nested included directories
excluded: configuration.excluded, // Always use the nested excluded directories
// The minimum warning threshold if both exist, otherwise the nested,
// and if it doesn't exist try to use the parent one
warningThreshold: warningThreshold.map { warningThreshold in
return min(configuration.warningThreshold ?? .max, warningThreshold)
} ?? configuration.warningThreshold,
reporter: reporter, // Always use the parent reporter
rules: mergingRules(with: configuration),
cachePath: cachePath, // Always use the parent cache path
rootPath: configuration.rootPath,
indentation: configuration.indentation
)
I couldn't agree more 😄. I filed #2856 to track writing comprehensive docs on configuration merging rules. As you familiarize yourself more with SwiftLint's behavior, this would be a great first ticket to contribute, if you're so inclined to help the next people going down the same path you did. No pressure of course. |
This is an implementation of merging nested
Configuration, based on the existing codebase (#676)When creating a merged
Configuration, the rule are:For the parameters:
includedandexcludedfolders of the nested configuration (with therootPathof the nested configuration)nilreportercachePathFor the rules:
opt_in_rules. Any rules specified underopt_in_rulesin the nested .swiftlint.yml will be enabled.disable_rules, in the same manner as described above.whitelist_rules, in which case only these rules will be enabled, and all others disabled.There is room for improvements, as I just used the existing
internal func merge(with configuration: Configuration) -> Configurationmethod to implement the changes.There should also be a way to override just part of a rule's configuration (rather than the whole configuration), but that requires deeper changes for a very specific use case, so it's not done in this PR.
I'll be adding some unit tests while this is being reviewed.
Let me know what you think!