Added "all" pseudo-rule for opt in rules - enables all opt in rules

[mildm8nnered]

Adds an "all" pseudo-rule - if specified in `opt_in_rules", this will enable all opt in rules.

For example:

opt_in_rules:
  - all

Any other entries in opt_in_rules will be ignored (e.g. duplicates will not be reported).

See https://github.com/realm/SwiftLint/issues/4540

[SwiftLintBot]

	18 Messages
:book:	Linting Aerial with this PR took 1.05s vs 1.03s on main (1% slower)
:book:	Linting Alamofire with this PR took 1.33s vs 1.33s on main (0% slower)
:book:	Linting Brave with this PR took 7.16s vs 7.16s on main (0% slower)
:book:	Linting DuckDuckGo with this PR took 3.0s vs 3.01s on main (0% faster)
:book:	Linting Firefox with this PR took 9.05s vs 9.06s on main (0% faster)
:book:	Linting Kickstarter with this PR took 10.02s vs 10.0s on main (0% slower)
:book:	Linting Moya with this PR took 0.51s vs 0.52s on main (1% faster)
:book:	Linting NetNewsWire with this PR took 2.95s vs 2.98s on main (1% faster)
:book:	Linting Nimble with this PR took 0.58s vs 0.58s on main (0% slower)
:book:	Linting PocketCasts with this PR took 7.0s vs 7.0s on main (0% slower)
:book:	Linting Quick with this PR took 0.22s vs 0.22s on main (0% slower)
:book:	Linting Realm with this PR took 11.3s vs 11.29s on main (0% slower)
:book:	Linting SourceKitten with this PR took 0.43s vs 0.42s on main (2% slower)
:book:	Linting Sourcery with this PR took 2.16s vs 2.16s on main (0% slower)
:book:	Linting Swift with this PR took 4.42s vs 4.43s on main (0% faster)
:book:	Linting VLC with this PR took 1.32s vs 1.31s on main (0% slower)
:book:	Linting Wire with this PR took 8.13s vs 8.14s on main (0% faster)
:book:	Linting WordPress with this PR took 10.83s vs 10.86s on main (0% faster)

Generated by :no_entry_sign: Danger

[mildm8nnered]

test failing because IO failure on output stream: No space left on device

[jpsim]

Thanks for the PR! I think this could be useful, but would like to hear your thoughts on my comment in https://github.com/realm/SwiftLint/issues/4540#issuecomment-1326448740 before I consider merging this.

test failing because IO failure on output stream: No space left on device

Our CI was having issues, which have since been fixed and the CI jobs now all passed. Sorry for the trouble.

[mildm8nnered]

I added a CHANGELOG notice, and a little bit of documentation to the README. I did see a Chinese and Korean README as well - not sure how you handle updating these ...

[SimplyDanny]

I added a CHANGELOG notice, and a little bit of documentation to the README. I did see a Chinese and Korean README as well - not sure how you handle updating these ...

You may safely ignore them. Some attentive users familiar with the languages will come by at some point to translate and update the README.

[mildm8nnered]

@SimplyDanny - I just spotted that I'm getting an "all" is not a valid rule identifier at runtime.

coming from Configuration.validate(ruleIds: Set<String>, valid: Set<String>, silent: Bool = false) -> Set<String>

I can suppress that by doing something like

optInRuleIdentifiers = validate(ruleIds: optInRuleIdentifiers, valid: validRuleIdentifiers.union(["all"]))

at the two call sites relating to optin rules, but that feels a tiny bit hacky. Any suggestions?

[SimplyDanny]

Adding all in all method calls is error prone. Either hard-code it in the method body or put it by default into invalidRuleIdsWarnedAbout. All not quite clean, but a special identifier requires special treatment. I currently have no better idea.

Thinking about this case, the disabled_rules section comes into my mind. We should also make sure that using all there leads to an error or a warning, because that wouldn't make any sense.

[mildm8nnered]

Adding all in all method calls is error prone. Either hard-code it in the method body or put it by default into invalidRuleIdsWarnedAbout. All not quite clean, but a special identifier requires special treatment. I currently have no better idea.

Thinking about this case, the disabled_rules section comes into my mind. We should also make sure that using all there leads to an error or a warning, because that wouldn't make any sense.

How about something like

        private func validateOptInRuleIdentifiers(ruleIds: Set<String>, valid: Set<String>, silent: Bool = false) -> Set<String> {
            validate(ruleIds: ruleIds, valid: valid.union(["all"]), silent: silent)
        }

[SimplyDanny]

Thinking about this case, the disabled_rules section comes into my mind. We should also make sure that using all there leads to an error or a warning, because that wouldn't make any sense.

Did you have a chance to think about this part, @mildm8nnered?

[mildm8nnered]

Thinking about this case, the disabled_rules section comes into my mind. We should also make sure that using all there leads to an error or a warning, because that wouldn't make any sense.

Did you have a chance to think about this part, @mildm8nnered?

So I think my last commit d88a5fb (which I've applied your suggestion to since) should handle that, as I'm only ignoring all for optin rules, so disabled_rules should still get a warning.

[SimplyDanny]

Thinking about this case, the disabled_rules section comes into my mind. We should also make sure that using all there leads to an error or a warning, because that wouldn't make any sense.

Did you have a chance to think about this part, @mildm8nnered?

So I think my last commit d88a5fb (which I've applied your suggestion to since) should handle that, as I'm only ignoring all for optin rules, so disabled_rules should still get a warning.

I see. Makes sense to me now. So the PR looks good to me now (again). I would still like to give @jpsim the chance to intervene. So let's wait for his feedback.

[mildm8nnered]

Is there some kind of general issue with the thread sanitiser checks?

I'm seeing similar failures on other PRs to the CI failures showing up on this one. For example, #4647 and #4657

[SimplyDanny]

Is there some kind of general issue with the thread sanitiser checks?

I first thought it was somehow caused by my change. When the second one failed too, it became clear that this must be a general issue. Now I see you have the same problem ...

[mildm8nnered]

I seem to be hitting thread sanitiser issues again on this branch :-(

[SimplyDanny]

The responsible job has been removed in #4701. Another rebase should fix your PR build.

[mildm8nnered]

Thanks @SimplyDanny - that got me a clean build again.

@jpsim - any thoughts on merging this?

[SimplyDanny]

Merged. 😊 Thanks again for all the work @mildm8nnered!

[shuguenot]

Has this been tested with nested configurations?

[mildm8nnered]

Has this been tested with nested configurations?

yes - are you seeing any problems?