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

Fixes warning about configurations for rules that are not enabled, when they are enabled in a parent config #4864

Merged

Conversation

mildm8nnered
Copy link
Collaborator

@mildm8nnered mildm8nnered commented Apr 4, 2023

Fixes #4858

The problem here is configurations like this:

included:
  - Plugins
  - Source
  - Tests
excluded:
  - Tests/SwiftLintFrameworkTests/Resources
opt_in_rules:
  - implicit_return
disabled_rules:
  - type_body_length
  - function_body_length
  - large_tuple

implicit_return:
  included:
    - closure

with child configurations like this:

parent_config: .swiftlint-parent.yml

implicit_return:
  included:
    - closure
    - function
    - getter

SwiftLint will report warning: Found a configuration for 'implicit_return' rule, but it is not enabled on 'opt_in_rules'. for the child config, even though it will correctly apply the new config.

We now pass in the parent configuration when validating rule configurations for the child, and do not complain about configurations where that rule is enabled in the parent.

There should be no change in behaviour at all here - just a change in the warnings.

Getting this correct turned out to be quite complex, but after this PR, it should be the case that whenever, and only when, a rule is disabled in the parent or "child" config, a warning message will be printed, and the exact warning will vary somewhat, depending on whether the rule is disabled in the parent or "child", or not enabled at all.

There is a separate issue with child_config that this PR does not attempt to address - See #5251

@mildm8nnered mildm8nnered changed the title Mildm8nnered fix some parent config issues Fixes warning about configurations for rules that are not enabled, when they are enabled in a parent config Apr 4, 2023
@SwiftLintBot
Copy link

SwiftLintBot commented Apr 4, 2023

17 Messages
📖 Linting Aerial with this PR took 1.22s vs 1.24s on main (1% faster)
📖 Linting Alamofire with this PR took 1.79s vs 1.81s on main (1% faster)
📖 Linting Brave with this PR took 10.36s vs 10.41s on main (0% faster)
📖 Linting DuckDuckGo with this PR took 5.52s vs 5.55s on main (0% faster)
📖 Linting Firefox with this PR took 12.78s vs 12.86s on main (0% faster)
📖 Linting Kickstarter with this PR took 12.43s vs 12.53s on main (0% faster)
📖 Linting Moya with this PR took 0.69s vs 0.69s on main (0% slower)
📖 Linting NetNewsWire with this PR took 3.66s vs 3.7s on main (1% faster)
📖 Linting Nimble with this PR took 1.01s vs 1.01s on main (0% slower)
📖 Linting PocketCasts with this PR took 10.15s vs 10.16s on main (0% faster)
📖 Linting Quick with this PR took 0.45s vs 0.46s on main (2% faster)
📖 Linting Realm with this PR took 6.36s vs 6.36s on main (0% slower)
📖 Linting Sourcery with this PR took 3.11s vs 3.13s on main (0% faster)
📖 Linting Swift with this PR took 6.21s vs 6.26s on main (0% faster)
📖 Linting VLC with this PR took 1.67s vs 1.68s on main (0% faster)
📖 Linting Wire with this PR took 23.31s vs 23.39s on main (0% faster)
📖 Linting WordPress with this PR took 15.2s vs 15.21s on main (0% faster)

Generated by 🚫 Danger

@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch from aaf59ed to a1a47ce Compare April 4, 2023 02:19
@mildm8nnered mildm8nnered marked this pull request as ready for review April 4, 2023 02:43
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.

I lost a bit the overview. Perhaps my comments/questions are stupid. But somehow the disabled rules must be taken into account, isn't it?

CHANGELOG.md Outdated Show resolved Hide resolved
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch from 60c3bed to 5a27b97 Compare April 5, 2023 22:39
@mildm8nnered
Copy link
Collaborator Author

I lost a bit the the overview. Perhaps my comments/questions are stupid. But somehow the disabled rules must be taken into account, isn't it?

I think you're right about disabled - I'll put together some local test cases to verify those, athough I'll try to find a configurable non-optin rule to test with because of #4859

@mildm8nnered mildm8nnered marked this pull request as draft April 6, 2023 00:07
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 4 times, most recently from 538b3b8 to 491bea8 Compare April 19, 2023 23:09
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 2 times, most recently from 2cccc14 to 9fb9619 Compare April 27, 2023 21:40
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 2 times, most recently from 270ae47 to ec30ced Compare June 27, 2023 08:21
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 2 times, most recently from fac976e to 0f10a37 Compare July 7, 2023 17:04
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 4 times, most recently from 4869293 to aa434ff Compare July 19, 2023 18:14
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch from aa434ff to 8314415 Compare July 26, 2023 21:09
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch from b53c0d6 to 75381d7 Compare August 6, 2023 17:13
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 4 times, most recently from 077c2bc to 8a3f4d1 Compare September 3, 2023 10:45
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch 2 times, most recently from 3405c7c to cf06542 Compare September 5, 2023 21:59
@mildm8nnered mildm8nnered force-pushed the mildm8nnered-fix-some-parent-config-issues branch from 03010b4 to 60a6162 Compare March 24, 2024 20:13
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.

Wow ... this has been a tough topic! After almost one year after this got opened, it will finally be merged. 😅

Thank you so much for your patience and the continuous improvements and rework!

@SimplyDanny SimplyDanny merged commit 5075cc0 into realm:main Mar 25, 2024
12 checks passed
@mildm8nnered
Copy link
Collaborator Author

Wow ... this has been a tough topic! After almost one year after this got opened, it will finally be merged. 😅

Thank you so much for your patience and the continuous improvements and rework!

No problem. We got there in the end :-)

@mildm8nnered mildm8nnered deleted the mildm8nnered-fix-some-parent-config-issues branch March 31, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants