Skip to content

Command-line option to disable merging of nested configurations#2245

Closed
aksvenk wants to merge 5 commits intorealm:masterfrom
aksvenk:feature/disable-nested-merge-commandline-option
Closed

Command-line option to disable merging of nested configurations#2245
aksvenk wants to merge 5 commits intorealm:masterfrom
aksvenk:feature/disable-nested-merge-commandline-option

Conversation

@aksvenk
Copy link
Copy Markdown

@aksvenk aksvenk commented Jun 16, 2018

This PR adds an option to disable merging configurations in nested folders.
Usage: swiftlint lint --disable-nested-config-merge

Motivation
Sometimes nested configurations in a project may have vastly different rules from the parent.
The current strategy of always merging nested configs might not cut it in the above case. The nested configs would inherit rules that are not expected from the parent.

Solution
A command-line flag has been added to disable merging of nested configurations. When the said flag is used, nested configurations are 'disconnected' from the parent and the rule-sets are not merged.

Potential use-case
The change in the project structure below:

  • allows nested config files to operate in a standalone manner without being affected by the root
  • allows the preparation of an aggregate SwiftLint report which includes the project and its frameworks by running the swiftlint command from Project A's root folder
Project A
   +-.swiftlint.yml (root)
   |
   +--+-Framework A
   |  +-.swiftlint.yml (nested)
   |
   +--+-Framework B
   |  +-.swiftlint.yml (nested)
   |
   +--+-Framework C
      +-.swiftlint.yml (nested)

Akshay Venkatesh added 4 commits June 16, 2018 18:45
Note: The `mergeNestedFlags` command-line option is registered on the `root` config only.
Nested configurations other than root will not have this value set.
@SwiftLintBot
Copy link
Copy Markdown

SwiftLintBot commented Jun 16, 2018

12 Messages
📖 Linting Aerial with this PR took 0.38s vs 0.36s on master (5% slower)
📖 Linting Alamofire with this PR took 3.12s vs 3.06s on master (1% slower)
📖 Linting Firefox with this PR took 11.41s vs 11.48s on master (0% faster)
📖 Linting Kickstarter with this PR took 16.9s vs 16.85s on master (0% slower)
📖 Linting Moya with this PR took 1.83s vs 1.85s on master (1% faster)
📖 Linting Nimble with this PR took 1.53s vs 1.54s on master (0% faster)
📖 Linting Quick with this PR took 0.49s vs 0.5s on master (2% faster)
📖 Linting Realm with this PR took 3.15s vs 3.13s on master (0% slower)
📖 Linting SourceKitten with this PR took 0.98s vs 0.98s on master (0% slower)
📖 Linting Sourcery with this PR took 4.32s vs 4.39s on master (1% faster)
📖 Linting Swift with this PR took 27.04s vs 27.42s on master (1% faster)
📖 Linting WordPress with this PR took 14.61s vs 14.63s on master (0% faster)

Generated by 🚫 Danger

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #2245 into master will decrease coverage by 0.03%.
The diff coverage is 29.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2245      +/-   ##
=========================================
- Coverage   92.13%   92.1%   -0.04%     
=========================================
  Files         285     285              
  Lines       14475   14486      +11     
=========================================
+ Hits        13337   13342       +5     
- Misses       1138    1144       +6
Impacted Files Coverage Δ
Source/swiftlint/Commands/AutoCorrectCommand.swift 0% <0%> (ø) ⬆️
...iftlint/Extensions/Configuration+CommandLine.swift 0% <0%> (ø) ⬆️
Source/swiftlint/Commands/LintCommand.swift 0% <0%> (ø) ⬆️
...ntFramework/Extensions/Configuration+Merging.swift 89.69% <100%> (+0.1%) ⬆️
...urce/SwiftLintFramework/Models/Configuration.swift 85.29% <83.33%> (-0.25%) ⬇️
Source/SwiftLintFramework/Models/Command.swift 98.7% <0%> (+1.29%) ⬆️

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 aeb7040...c6f3481. Read the comment docs.

@jpsim
Copy link
Copy Markdown
Collaborator

jpsim commented Nov 27, 2018

Thanks for the PR here and sorry for not reviewing sooner. I definitely see the value of disconnecting a nested configuration file from its parents. However, I still think it's useful to make this decision on a per-configuration basis rather than based on the invocation of swiftlint. Besides, I think a useful property of SwiftLint configurations is that you can go to any Swift project that has properly configured SwiftLint and run swiftlint in its root and have that do the right thing.

So instead of a CLI flag, how about we add a Configuration key like root: true or root_configuration: true? In my mind, this would "reset" the nesting and further subdirectories would merge with all parent configurations up to its closest "root".

What do you think?

@Jeehut
Copy link
Copy Markdown
Collaborator

Jeehut commented Feb 25, 2019

@aksvenk Are you still interested in this feature? If so, would you mind answering @jpsim's question? Just trying to bring the discussion forward.

@aksvenk
Copy link
Copy Markdown
Author

aksvenk commented Feb 26, 2019

@Dschee sorry about the delay.

@jpsim I get your point, it is heaps more elegant to stop nesting through configuration than command-line arguments. We do gain the capability of locking in a configuration to "what the author intended" on third party code while keeping the nesting behaviour of the subdirectories untouched.

The initial reasoning behind the command-line parameter was to provide this feature instantly to projects that don't have control over code that they nest, but I do agree it is a jackhammer approach.

I'm happy to close this and open another PR with the configuration file based flag.

Do you think it is worth somehow showing the user where the merging of configurations was terminated in each path so as to avoid confusions about rules not being picked up in configurations "outside the root". Or does this feature already exist?

@aksvenk aksvenk closed this Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants