Is blanket_disable_command really necessary?

[tomhuettmann]

New Issue Checklist

• [x] Updated SwiftLint to the latest version (updated via brew, brew currently still uses 0.53.0)
• [x] I searched for existing GitHub issues

Question / New rule request

I stumbled across the paragraph in the README.md:L432-433:

... The rules will be disabled until the end of the file or until the linter sees a matching enable comment: ...

and see a collision with the in https://github.com/realm/SwiftLint/pull/4731 introduced blanket_disable_command rule (https://realm.github.io/SwiftLint/blanket_disable_command.html).

My question now is if re-enabling rules before the end of file is really necessary or not? I tried it out in a simple project, and it seems like written in the README.md, all the disabled files are automatically enabled again before linting the next file.

If so, I would like to recommend a new rule to get rid of those introduced unnecessary enable commands at file endings:

no_swiftlint_enable_at_end_of_file:
  name: 'No SwiftLint enable at end of file'
  message: 'There is no swiftlint:enable necessary at the end of a file'
  regex: 'swiftlint:enable[^\n]*\n*\Z'

Non triggering example:

// swiftlint:disable foo
... some code ...
// swiftlint:enable foo
... some code ...

Triggering example:

// swiftlint:disable foo
... some code ...
// swiftlint:enable foo

, where the // swiftlint:enable foo is the last thing before EOF.

[SimplyDanny]

The idea behind the blanket_disable_command rule is like: People might just write // swiftlint:disable while they should use // swiftlint:disable:this, // swiftlint:disable:next or // swiftlint:disable:previous to be very explicit about the location to ignore; to make this a real exception. The rule assumes, a blanket // swiftlint:disable is unintentional and so notifies users about it.

Enabling a disabled rule at the end of the file is one way to circumvent blanket_disable_command from triggering in the whole file. Another one is

// swiftlint:disable:next blanket_disable_command
// swiftlint:disable nesting

at the beginning of the file.

I like your idea to complain about enabled rules at the end of files. The recommended alternative would be to disable the rule consciously at the beginning of files. This could be an option for blanket_disable_command.

@mildm8nnered: You are the expert for blanket_disable_command. What do you think?

[mildm8nnered]

"My question now is if re-enabling rules before the end of file is really necessary or not? " - really we want users to re-enable the rule as soon as possible ideally by using this, next, or previous constructs, which are line-scoped, or by explicitly re-enabling the rule right after the rule violations - re-enabling the rule at the end of the file will satisfy blanket_disable_command, but definitely is a bit of a bad smell.

If you find yourself re-enabling rules at the end of the file, instead of limiting the scope, you are effectively "circumventing" the rule, as @SimplyDanny points out above.

In cases where you need or want to disable the rule across the entire file (e.g. some third-party code that doesn't follow your normal standards across the whole file), a better way to do this is to use the

// swiftlint:disable:next blanket_disable_command
// swiftlint:disable nesting

construct that @SimplyDanny mentioned, or, if in your codebase you have specific rules that you always want to treat as file-scoped, you can specify those via the allowed_rules configuration parameter (by default, file_header, file_length, file_name, file_name_no_space and single_test_class will be ignored).

I could see a 'strict' option to blanket_disable_command that looked for enables just before EOF, and warned about them, advising users to use one of the other constructs instead having some values although I'm not sure that was @tomhuettmann 's original intent.

[tomhuettmann]

Thanks for your effort and your explanations @SimplyDanny and @mildm8nnered! My first impression was only that it is a bit weird that blanket_disable_command warns you about something that is technically not needed. I get now that the message rule should be re-enabled before the end of the file is rather a hint to re-enabled rules as soon as possible than a technical requirement.

Maybe it would help others for future clarification to adjust the error messages for enables just before EOF a bit mentioning the idea behind it. But I am also fine with closing this right now. Thanks once more! ❤️

[SimplyDanny]

I think we can go with a new option. Having it in the documentation should already make a bit clearer what the idea of the rule is.