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

Feature Request: Mechanism to enable all opt-in rules by default #4540

Closed
2 tasks done
mildm8nnered opened this issue Nov 9, 2022 · 6 comments
Closed
2 tasks done
Labels
enhancement Ideas for improvements of existing features and rules.

Comments

@mildm8nnered
Copy link
Collaborator

New Issue Checklist

Describe the bug

I wasn't sure whether to file this as a new rule request or not. I guess an all "pseudo-rule" would do what I want.

I'd like to be able to enable all opt-in rules in my configuration file;

opt_in_rules:
  - all

would be fine (then I can disable the ones I don't want via disabled_rules).

This would save me having to check for new opt-in rules when I upgrade to a new version of SwiftLint

Complete output when running SwiftLint, including the stack trace and command used

N/A

Environment

N/A

@SimplyDanny SimplyDanny added the enhancement Ideas for improvements of existing features and rules. label Nov 9, 2022
@mildm8nnered
Copy link
Collaborator Author

See #4544

@jpsim
Copy link
Collaborator

jpsim commented Nov 12, 2022

Have you tried the --enable-all-rules CLI flag? Does that do what you want?

By the way, we’ve historically been pretty liberal with allowing opt-in rules so I wouldn’t recommend actually keeping all rules enabled.

@mildm8nnered
Copy link
Collaborator Author

mildm8nnered commented Nov 13, 2022

So --enable-all-rules does not do what I want, because it ignores disabled_rules:, and in practice, there are almost no cases where a user would really want all rules enabled.

The intent here is to enable all opt-in rules by default, but to allow the user to disable any that they don't want.

Our internal policy (at my place of work) is to pretty much enable all rules (and all new rules), except any that we find annoying/disagree with the premise of/currently produce too many hits in our codebase (we operate a zero warnings policy).

When we upgrade SwiftLint, we use swiftlint rules to detect un-enabled opt-in rules, and decide whether to include them or not.

This is annoying because we can't tell from rules output whether an un-enabled rule is actually new, or whether it's an older rule that we evaluated previously and decided not to enable. As a hacky workaround for this, we actually list all opt-in rules in opt_in_rules:, and comment out those we don't want, but there's still a process of discovery and reconciliation to go through.

With all, any new opt-in rules in a new version of SwiftLint would be picked up automatically, with no need for us to discover them from rules output or the release notes (and our config would be a lot simpler).

We can then just run SwiftLint, and decide, based on any hits, whether to keep the new rules (and patch any violations), or to explicitly disable them.

@jpsim
Copy link
Collaborator

jpsim commented Nov 24, 2022

So --enable-all-rules does not do what I want, because it ignores disabled_rules:, and in practice, there are almost no cases where a user would really want all rules enabled.

Yeah, mostly it's in place for testing SwiftLint itself rather than a frequently used end-user feature.

We use it to run on PRs to detect what changes there are to the lint results of linting dozens of open source projects.

It's also useful when someone reports an issue, they can lint a file with the flag and get the violations from all rules, regardless of what's defined in their .swiftlint.yml config file.

With all, any new opt-in rules in a new version of SwiftLint would be picked up automatically, with no need for us to discover them from rules output or the release notes (and our config would be a lot simpler).

Wanting to audit new rules is a common workflow and one for which we've never had a good story other than manually scanning the release notes.

Having an all pseudo-rule like this would be one way to help improve this workflow.

Another one could be to track version info with rules, like add a introducedIn: "0.50.1" parameter to RuleDescription so you could run swiftlint rules --introduced-after 0.50.0 and get all the new rules since you last audited rules / updated SwiftLint.

This would help users who don't want to explicitly all the rules they don't want in the disabled: section of their SwiftLint config.

Like for Lyft's case, we use only_rules instead of opt_in_rules / disabled_rules so we wouldn't benefit from an all pseudo-rule, but we would benefit from the version tracking I mentioned.

What do you think?

@mildm8nnered
Copy link
Collaborator Author

So I like the all option I've suggested because it's quite a small change (see https://github.com/realm/SwiftLint/pull/4544/files), and would bring a lot of value for my particular use case.

I can see the value of introducedIn: as well for other users, but that feels like it would add some overhead back porting it to existing rules, and another thing to do for any new rules.

Could we have both?

@mildm8nnered
Copy link
Collaborator Author

Resolved by #4544

417-72KI added a commit to 417-72KI/MultipartFormDataParser that referenced this issue Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

No branches or pull requests

3 participants