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

The superfluous_disable_command rule should report disabled rules too #5715

Open
2 tasks done
revolter opened this issue Jul 30, 2024 · 5 comments
Open
2 tasks done
Labels
enhancement Ideas for improvements of existing features and rules. rule-request Requests for a new rules.

Comments

@revolter
Copy link
Contributor

New Issue Checklist

Describe the bug

If you disable a <rule> using the disabled_rules configuration key, but still have // swiftlint:disable <rule> in the code, the superfluous_disable_command rule should still report a warning for them.

I didn't complete the rest of the template, because I think it's more of a logic issue than a rule issue, and it's pretty self explanatory, but let me know if you want me to do that.

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

Environment

  • SwiftLint version (run swiftlint version to be sure)?
  • Installation method used (Homebrew, CocoaPods, building from source, etc)?
  • Paste your configuration file:
# insert yaml contents here
  • Are you using nested configurations?
    If so, paste their relative paths and respective contents.
  • Which Xcode version are you using (check xcodebuild -version)?
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.
// This triggers a violation:
let foo = try! bar()
@SimplyDanny SimplyDanny added the enhancement Ideas for improvements of existing features and rules. label Jul 30, 2024
@SimplyDanny
Copy link
Collaborator

Seems like this has never been intended with this rule. However, this sounds like a reasonable enhancement of the current logic. A dedicated rule for disable/enable commands without a matching active rule would also be an option. Perhaps this is even better to really match all command types (not only commands to disable the rule).

@mildm8nnered
Copy link
Collaborator

So I think the existing behaviour is kind of intentional, and has some advantages.

For example, consider analyzer rules - we do want to be able to suppress false alarms (or stuff we can't be bothered to fix), but if we got a superfluous_disable_command warning when running linting, that would not be helpful.

Of course that's just a happy accident of the current behaviour, and we could always special case analyzer rules, but in the general case, if I turn some rules off, for whatever reason, it's not helpful to then get a bunch of superfluous_disable_command warnings

@SimplyDanny
Copy link
Collaborator

These are good arguments for a dedicated rule.

@SimplyDanny SimplyDanny added rule-request Requests for a new rules. and removed enhancement Ideas for improvements of existing features and rules. labels Jul 31, 2024
@mildm8nnered
Copy link
Collaborator

These are good arguments for a dedicated rule.

I think my natural inclination is to say a configuration option feels better somehow. Especially in this case - superfluous_disable_command is a bit of a special case - it's kind of a "meta" rule that gets implemented in the Linter.

I'm really struggling with that right now in #5670

The non-discoverability of options versus rules make making it a rule quite appealing, but I think what we need to do there is improve the discoverability and documentation of options.

@SimplyDanny
Copy link
Collaborator

An option is fine for me as well.

@SimplyDanny SimplyDanny added the enhancement Ideas for improvements of existing features and rules. label Aug 2, 2024
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. rule-request Requests for a new rules.
Projects
None yet
Development

No branches or pull requests

3 participants