Fix `--only-rule` config issues #5773

mildm8nnered · 2024-08-28T01:26:51Z

Addresses #5711, where if there was no default configuration file, --only-rule would be ignored.

Replaces #5725

Previously, --only-rule some_rule was implemented as though the configuration was:

only_rules:
    - some_rule

This did not interact well when the top level configuration was absent, or with child configurations.

This PR adds a new RulesMode enum - to represent --only-rule: onlyCommandLine, and the other enums have been renamed to make it clearer whether they are derived from the configuration file or the command line.

This allows interactions with child configurations to be applied correctly.

Like .allCommandLine (previously allEnabled), rules enabled with --only-rule can still be disabled in a child configuration, so if there are specific directories that the rule should not run on, that is supported. This may be useful when using --fix with --only-rule

Additionally, --only-rule can now be specified more than once, to enable multiple rules

SwiftLintBot · 2024-08-28T01:31:56Z

	17 Messages
📖	Linting Aerial with this PR took 0.92s vs 0.92s on main (0% slower)
📖	Linting Alamofire with this PR took 1.26s vs 1.27s on main (0% faster)
📖	Linting Brave with this PR took 7.17s vs 7.19s on main (0% faster)
📖	Linting DuckDuckGo with this PR took 5.05s vs 5.05s on main (0% slower)
📖	Linting Firefox with this PR took 10.56s vs 10.58s on main (0% faster)
📖	Linting Kickstarter with this PR took 9.69s vs 9.81s on main (1% faster)
📖	Linting Moya with this PR took 0.53s vs 0.53s on main (0% slower)
📖	Linting NetNewsWire with this PR took 2.62s vs 2.62s on main (0% slower)
📖	Linting Nimble with this PR took 0.77s vs 0.77s on main (0% slower)
📖	Linting PocketCasts with this PR took 8.53s vs 8.41s on main (1% slower)
📖	Linting Quick with this PR took 0.44s vs 0.46s on main (4% faster)
📖	Linting Realm with this PR took 4.49s vs 4.47s on main (0% slower)
📖	Linting Sourcery with this PR took 2.3s vs 2.31s on main (0% faster)
📖	Linting Swift with this PR took 4.48s vs 4.47s on main (0% slower)
📖	Linting VLC with this PR took 1.25s vs 1.25s on main (0% slower)
📖	Linting Wire with this PR took 17.44s vs 17.39s on main (0% slower)
📖	Linting WordPress with this PR took 11.51s vs 11.47s on main (0% slower)

Generated by 🚫 Danger

SimplyDanny · 2024-10-16T20:50:08Z

Source/SwiftLintCore/Extensions/Configuration+RulesMode.swift

@@ -26,6 +26,10 @@ public extension Configuration {
        /// Only enable the rules explicitly listed.
        case only(Set<String>)

+        /// Only enable the rule explicitly listed on the command line. The rule may have multiple identifiers,
+        /// hence why this is represented as a Set


Suggested change

/// hence why this is represented as a Set

/// hence why this is represented as a set.

I've changed this one and the one above to clarify the difference between onlyRules and onlyRule

/// Only enable the rules explicitly listed in the configuration files. case only(Set<String>) /// Only enable the rule explicitly listed on the command line (and it's aliases). case onlyRule(Set<String>)

It would be nice if the naming reflected that a bit more, but I don't have any good suggestions.

SimplyDanny · 2024-10-19T21:55:12Z

Source/SwiftLintCore/Extensions/Configuration+RulesWrapper.swift

+                onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers)
+                resultingRules = allRulesWrapped.filter { tuple in
+                    onlyRulesRuleIdentifiers.contains(type(of: tuple.rule).description.identifier)
+                }.map(\.rule)


Can we avoid duplicating the code from the previous case? Does Swift allow case var .a(x), .b(x)?

So weirdly

case var .only(onlyRulesRuleIdentifiers), var .onlyRule(onlyRulesRuleIdentifiers): customRulesFilter = { onlyRulesRuleIdentifiers.contains($0.identifier) } onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers) resultingRules = allRulesWrapped.filter { tuple in onlyRulesRuleIdentifiers.contains(type(of: tuple.rule).description.identifier) }.map(\.rule)

would not compile for me. It did not like the onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers) line. Digging into that a little separately.

but this works:

case let .only(onlyRulesRuleIdentifiers), let .onlyRule(onlyRulesRuleIdentifiers): customRulesFilter = { onlyRulesRuleIdentifiers.contains($0.identifier) } let onlyRulesRuleIdentifiers = validate(ruleIds: onlyRulesRuleIdentifiers, valid: validRuleIdentifiers) resultingRules = allRulesWrapped.filter { tuple in onlyRulesRuleIdentifiers.contains(type(of: tuple.rule).description.identifier) }.map(\.rule)

https://dev.azure.com/jpsim/SwiftLint/_build/results?buildId=9464&view=logs&j=93056758-5bfb-5750-f113-e720ddefdb4c&t=752f629f-ba5f-5dee-dc9b-30ab3665ce7f

Oh, that means the Swift 5 compiler crashes while the Swift 6 compiler builds it just fine?

That was my reading of it as well.

SimplyDanny · 2024-10-19T21:59:52Z

Source/SwiftLintCore/Extensions/Configuration+RulesMode.swift

@@ -101,6 +104,9 @@ public extension Configuration {
            case let .only(onlyRules):
                return .only(Set(onlyRules.map(aliasResolver)))

+            case let .onlyRule(onlyRules):
+                return .onlyRule(Set(onlyRules.map(aliasResolver)))


The .onlyRule case having multiple identifiers associated looks confusing. It's only here, when the aliases come into the game. Could we map to .only here instead and forget about .onlyRule from now on?

So that would be nicer, and the original code tried to implement --only-rule via .only. The problem is that when we're merging any later configurations, generally we want --only-rule to override them, as it's been specified on the command line, whereas with only_rules, the child configuration should take precedence.

Totally agree about the multiple identifiers - I do have a follow-on PR that allows --only-rule to be specified multiple times on the command line, to enable multiple rules, so it's slightly more understandable in that context.

It would be better if the naming were clearer - both allEnabled and onlyRule are really command line options, and it would be nice if their naming captured that somehow, or at least if onlyRule was somehow more distinctive, but I don't have any elegant suggestions.

I see. Perhaps we could be more specific with the mode names having for example .defaultConfiguration, .onlyConfiguration, .onlyCommandLine and .allCommandLine.

Allowing multiple --only-rule command line arguments makes it more reasonable that .onlyRule takes a set of IDs. Let's document that at its declaration as well.

SimplyDanny

Very much looking forward to get this!

Naveen-C-Ramachandrappa approved these changes Aug 28, 2024

View reviewed changes

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch from 39d5e73 to f21a455 Compare August 31, 2024 13:04

mildm8nnered mentioned this pull request Sep 2, 2024

Fix only rule config issues #5725

Closed

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch 3 times, most recently from fcc3e1d to e7abb8a Compare September 8, 2024 13:24

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch 3 times, most recently from 635bce7 to 8fcdf52 Compare September 21, 2024 18:47

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch 2 times, most recently from 2620ae0 to a4d2e1f Compare October 5, 2024 17:26

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch 2 times, most recently from c860708 to f3014c7 Compare October 11, 2024 18:26

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch from f3014c7 to 64ebe80 Compare October 15, 2024 18:17

mildm8nnered mentioned this pull request Oct 15, 2024

Allow multiple -only_rule command line arguments #5826

Closed

mildm8nnered marked this pull request as ready for review October 18, 2024 03:02

SimplyDanny reviewed Oct 19, 2024

View reviewed changes

mildm8nnered added 8 commits October 20, 2024 12:01

First pass

ff2da0c

Added some unit tests

4ea0803

More fixes

6bf57a9

Fixed line length

582dc38

Allow rules to be disabled in a child config

c6acb8a

Fixed unit test now that rules can be disabled in a child

1d4c989

Added

4475218

Tweaked comments

7a01856

mildm8nnered force-pushed the mildm8nnered-fix-only-rule-config-issues-properly branch from e708342 to 7a01856 Compare October 20, 2024 11:01

mildm8nnered added 3 commits October 20, 2024 12:05

Clarified comments

345281d

The compiler did not like this locally for me

15faf28

Fixed compilation

e97e049

mildm8nnered added 3 commits October 25, 2024 01:57

Renamed RulesMode enums

9c7d573

Enable multiple --only-rule invocations

3835e7c

Updated comment

120cf44

SimplyDanny approved these changes Oct 25, 2024

View reviewed changes

Improved help

e7e7d31

mildm8nnered merged commit 9ea4374 into realm:main Oct 25, 2024
14 checks passed

mildm8nnered deleted the mildm8nnered-fix-only-rule-config-issues-properly branch October 25, 2024 16:19

mildm8nnered mentioned this pull request Nov 16, 2024

--only-rule option doesn't work without a .swiftlint.yml configuration #5711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `--only-rule` config issues #5773

Fix `--only-rule` config issues #5773

mildm8nnered commented Aug 28, 2024 •

edited

Loading

SwiftLintBot commented Aug 28, 2024 •

edited

Loading

SimplyDanny Oct 16, 2024

mildm8nnered Oct 20, 2024

SimplyDanny Oct 19, 2024

mildm8nnered Oct 20, 2024

mildm8nnered Oct 20, 2024

SimplyDanny Oct 24, 2024

mildm8nnered Oct 25, 2024

SimplyDanny Oct 19, 2024

mildm8nnered Oct 20, 2024

SimplyDanny Oct 24, 2024

SimplyDanny left a comment

	/// hence why this is represented as a Set
	/// hence why this is represented as a set.

Fix --only-rule config issues #5773

Fix --only-rule config issues #5773

Conversation

mildm8nnered commented Aug 28, 2024 • edited Loading

SwiftLintBot commented Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimplyDanny left a comment

Choose a reason for hiding this comment

Fix `--only-rule` config issues #5773

Fix `--only-rule` config issues #5773

mildm8nnered commented Aug 28, 2024 •

edited

Loading

SwiftLintBot commented Aug 28, 2024 •

edited

Loading