Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disabled rules in child configs are no longer automatically ignored if they are opt-in #4863

Conversation

mildm8nnered
Copy link
Collaborator

@mildm8nnered mildm8nnered commented Apr 4, 2023

Fixes some unexpected behaviors in the interaction between parent and child configurations.

For example, if a rule is enabled and disabled in the parent config, and not mentioned in the child config (case 3 below), currently the rule will become re-enabled in the merged configuration.

Resolves #4876, which documents the unexpected behaviors.

Below is the truth table for whether an optin rule is enabled in the child configuration, as a function of whether it is listed in the opt_in_rules, and/or the disabled_rules sections of the parent and child configuration.

current is the value for main/0.51.0, and new is the value produced by this branch.

asterisked rows have changed.

        parent    parent      child      child
        opt_in   disabled    opt_in     disabled    current       new

0       false      false      false      false       false       false
1        true      false      false      false        true        true
2       false       true      false      false       false       false
3        true       true      false      false        true       false *
4       false      false       true      false        true        true
5        true      false       true      false        true        true
6       false       true       true      false        true        true
7        true       true       true      false        true        true
8       false      false      false       true       false       false
9        true      false      false       true       false       false
10      false       true      false       true       false       false
11       true       true      false       true       false       false
12      false      false       true       true        true       false *
13       true      false       true       true        true       false *
14      false       true       true       true        true       false *
15       true       true       true       true        true       false *

To take case 3 in detail, if an optin rule is listed in opt_in_rules and disabled_rules in the parent configuration, and not mentioned in the child, then currently (in main/0.51.0) the rule will be disabled for the parent configuration, but enabled in the child.

parent.yml:

opt_in_rules:
  - implicit_return
disabled_rules:
  - implicit_return

child.yml:

parent_config: parent.yml

checking enablement:

% swiftlint rules  --config parent.yml | grep implicit_return
| implicit_return                          | yes    | yes         | NO                     | style       | no       | yes            | severity: ... |
% swiftlint rules  --config child.yml | grep implicit_return
| implicit_return                          | yes    | yes         | YES                    | style       | no       | yes            | severity: ... |

and actual behaviour, given:

class ImplicitReturn {
   var foo: Int {
       return 0
   }
}
 % swiftlint lint  --config parent.yml ImplicitReturn.swift 
Linting Swift files at paths ImplicitReturn.swift
Linting 'ImplicitReturn.swift' (1/1)
Done linting! Found 0 violations, 0 serious in 1 file.
%
% swiftlint lint --config child.yml ImplicitReturn.swift 
Linting Swift files at paths ImplicitReturn.swift
Linting 'ImplicitReturn.swift' (1/1)
/Users/martin.redington/Documents/Hudl/Source/SwiftLint/ImplicitReturn.swift:3:8: warning: Implicit Return Violation: Prefer implicit returns in closures, functions and getters (implicit_return)
Done linting! Found 1 violation, 0 serious in 1 file.

Similarly, in cases 12 through 15, the rule is both mentioned in the child's opt_in_rules, and disabled_rules, which should surely mean the rule ends up disabled in the child, no matter what the parent says.

This PR changes the processing of these configurations, to produce the truth values shown in the new column above, which seem more sensible than the current values.

There is a related issue when the parent has only rules, and the child has a default configuration.

Here is the truth table for this case:

       child       child
       optin     disabled    current     new

0      false      false       true       true
1       true      false       true       true
2      false       true      false      false
3       true       true       true      false *

Examining case 3

parent.yml:

only_rules:
   - implicit_return

and child.yml:

parent_config: parent.yml

opt_in_rules:
  - implicit_return
disabled_rules:
  - implicit_return

Currently (in main/0.51.0) the rule would end up enabled for the child, even though it is explicitly disabled in the child configuration.

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 4, 2023

17 Messages
📖 Linting Aerial with this PR took 1.11s vs 1.11s on main (0% slower)
📖 Linting Alamofire with this PR took 1.45s vs 1.43s on main (1% slower)
📖 Linting Brave with this PR took 8.08s vs 8.08s on main (0% slower)
📖 Linting DuckDuckGo with this PR took 3.93s vs 3.94s on main (0% faster)
📖 Linting Firefox with this PR took 9.53s vs 9.52s on main (0% slower)
📖 Linting Kickstarter with this PR took 10.37s vs 10.34s on main (0% slower)
📖 Linting Moya with this PR took 0.59s vs 0.59s on main (0% slower)
📖 Linting NetNewsWire with this PR took 3.11s vs 3.09s on main (0% slower)
📖 Linting Nimble with this PR took 0.76s vs 0.76s on main (0% slower)
📖 Linting PocketCasts with this PR took 8.04s vs 8.01s on main (0% slower)
📖 Linting Quick with this PR took 0.38s vs 0.37s on main (2% slower)
📖 Linting Realm with this PR took 11.17s vs 11.15s on main (0% slower)
📖 Linting Sourcery with this PR took 2.31s vs 2.3s on main (0% slower)
📖 Linting Swift with this PR took 5.19s vs 5.16s on main (0% slower)
📖 Linting VLC with this PR took 1.41s vs 1.4s on main (0% slower)
📖 Linting Wire with this PR took 8.55s vs 8.53s on main (0% slower)
📖 Linting WordPress with this PR took 12.35s vs 12.36s on main (0% faster)

Generated by 🚫 Danger

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch from c4c2571 to ac7a69a Compare April 5, 2023 22:38
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch 4 times, most recently from a5b6e05 to 843ab68 Compare April 19, 2023 23:08
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch 2 times, most recently from b66bc90 to faab0f7 Compare April 27, 2023 21:39
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch from faab0f7 to 51e9533 Compare May 30, 2023 21:05
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch 3 times, most recently from c075dd7 to e8e4b46 Compare June 24, 2023 11:19
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch from e8e4b46 to e70f529 Compare June 27, 2023 08:21
Copy link
Collaborator

@SimplyDanny SimplyDanny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me! However, I'm having a hard time following the tests. 🫣

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch 2 times, most recently from fca516a to 112bdcd Compare July 7, 2023 17:02
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch 4 times, most recently from c360b55 to bb848df Compare July 19, 2023 18:13
@mildm8nnered mildm8nnered marked this pull request as ready for review July 19, 2023 21:38
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-disabled-rules-in-child-config branch from 4666fe2 to 0932089 Compare July 25, 2023 01:53
@SimplyDanny SimplyDanny merged commit 9a5c3ed into realm:main Jul 26, 2023
@SimplyDanny
Copy link
Collaborator

Thanks for all the effort, @mildm8nnered!

@mildm8nnered mildm8nnered deleted the mildm8nnered-fix-disabled-rules-in-child-config branch October 1, 2023 10:42
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.

Question: What is the expected behaviour of opt_in_rules and disabled_rules in a child configuration?
3 participants