only rule should work for individual custom rules#6057
Merged
mildm8nnered merged 23 commits intorealm:mainfrom Sep 18, 2025
Merged
only rule should work for individual custom rules#6057mildm8nnered merged 23 commits intorealm:mainfrom
mildm8nnered merged 23 commits intorealm:mainfrom
Conversation
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issues #6029 and #6058 by allowing individual custom rules to be enabled via the only_rules configuration (or the --only-rule command line option) without requiring a separate custom_rules configuration. It also adds a convenience property for retrieving CustomRules, sorts custom rule violations deterministically, and updates tests/documentation accordingly.
- Enables specifying individual custom rules in only_rules.
- Introduces lazy evaluation of valid rule identifiers and a new extension for accessing CustomRules.
- Ensures deterministic ordering of custom rule violations.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/FrameworkTests/CustomRulesTests.swift | Adds tests for only_rules support with custom rules enabling |
| Tests/FrameworkTests/ConfigurationTests.swift | Adds test verifying specific custom rules behavior in the configuration |
| Source/SwiftLintFramework/Rules/CustomRules.swift | Sorts customRuleConfigurations to guarantee deterministic violation order |
| Source/SwiftLintFramework/Configuration/Configuration+RulesWrapper.swift | Refactors rule identifier handling and customRules appending logic |
| Source/SwiftLintFramework/Configuration/Configuration+RulesMode.swift | Updates CustomRules handling in rules mode configurations |
| Source/SwiftLintFramework/Configuration/Configuration+Parsing.swift | Updates parsing and validation to support custom rule identifiers |
| CHANGELOG.md | Documents the bug fix and behavior change for custom_rules configuration |
mildm8nnered
commented
Apr 22, 2025
| private let aliasResolver: (String) -> String | ||
|
|
||
| private var invalidRuleIdsWarnedAbout: Set<String> = [] | ||
| private var customRulesIdentifiers: Set<String> { |
Collaborator
Author
There was a problem hiding this comment.
I had these as lazy initially, which should be safe, as resultingRules is locked, and the only other place they are called from is when we're merging configurations, but making them non-lazy seemed safer.
e07e075 to
04d22aa
Compare
This was
linked to
issues
Apr 23, 2025
04d22aa to
497fe5f
Compare
497fe5f to
16aafde
Compare
SimplyDanny
approved these changes
Jul 23, 2025
Collaborator
SimplyDanny
left a comment
There was a problem hiding this comment.
Just a nit. Otherwise looks good to me!
Comment on lines
+163
to
+168
| guard let resultingCustomRules = config.rules.first(where: { $0 is CustomRules }) as? CustomRules | ||
| else { | ||
| XCTFail("Custom rules are expected to be present") | ||
| return | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| guard let resultingCustomRules = config.rules.first(where: { $0 is CustomRules }) as? CustomRules | |
| else { | |
| XCTFail("Custom rules are expected to be present") | |
| return | |
| } | |
| let resultingCustomRules = config.rules.customRules | |
| XCTAssertNotNil(resultingCustomRules) |
Collaborator
Author
There was a problem hiding this comment.
So the existing extension on [ConfigurationRuleWrapper] was not quite right here - it retrieves the custom rules before disablement, but I added a similar extension for [any Rule], which enables config.rules.customRules and retrieves the configured rules, and replaces eight cases of rules.first(where: { $0 is CustomRules }) as? CustomRules in the other tests.
16aafde to
6f84579
Compare
c8434bd to
befab0b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #6029 and #6058
Allows individual custom rules to be specified in the
only_rulesconfiguration, or the--only-rulecommand line option, without also having to specifycustom_rules.I think this may bring individual custom_rules to be almost equivalent to "normal" rules, as they are now supported in
swiftlint:disablecommands, and most other cases as well.The key change here is that when we calculate the resulting rules, if CustomRules is not in there, we manually add it in.
There was already pre-existing code later on to filter which custom rules are enabled.
Some further changes were required to make sure that the warning about configured but not enabled rules was fired for this configuration in the
only_rulescase.Other changes
We were calling
(allRulesWrapped.first { $0.rule is CustomRules })?.rule as? CustomRulesin about five different places, so I added a conveniencevarto[ConfigurationRuleWrapper].validRuleIdentifiersis now calculated lazily.Order of custom_rules violations
The order of violations reported by custom_rules was not deterministic - see #6058 - violations are now reported sorted by the custom rule identifier.
There is still some weirdness in the order that violations are output in, but I'll raise those issues elsewhere - they should at least be deterministic now.