search

r-lib / lintr

Static Code Analysis for R
https://lintr.r-lib.org
Other
1.2k stars 187 forks source link

New linter for unqualified `# nolint` or `# nolint start` #690

Open AshesITR opened 3 years ago

AshesITR commented 3 years ago

Once #660 is in master, we support excluding specific linters. A linter linting exclusion comments without a list of excluded linters would be nice to have.

Ideally it would say something like

Use # nolint with a list of excluded linters, e.g. # nolint: xxx, yyy.

Where the list of linters in the message is auto-generated by checking which of the configured linters produce a hit. Might be hard to do since exclusions would apply to the linter and thus mute it.

AshesITR commented 3 years ago

Just a few thoughts on how we might support this:

  1. Add a new attribute important to Lint, defaulting to FALSE.
  2. Make exclude_lints not exclude lints with important = TRUE unless (a) the entire file is excluded or (b) the specific linter is excluded

(a) would be hard to change because we currently optimize away linting completely excluded files. (b) at least allows the user to mute important lints by explicitly specifying it in the # nolint: important_linter. way. Otherwise there would be no way to disable a specific instance of an important lint without excluding the entire file or the linter.

MichaelChirico commented 3 years ago

A different way to go would be to add an option recognized in .lintr, then throw a warning() from exclude that an unqualified # nolint was found and is being ignored

AshesITR commented 3 years ago

I could see both working and since this would probably be the only instance of an "important" lint, your approach seems more elegant. I'm not sure whether we want to open WARNINGs as a secondary information channel though, downstream code might not expect warnings from lintr.

MichaelChirico commented 3 years ago

Default would be for it to be off (at least for now), so no warnings unless intentionally toggled

AshesITR commented 3 years ago

I think it might make sense to turn it on to increase awareness for the new specific exclusions. However if we trigger warnings, this might break existing code that only expects a list of lints. So if we decide to implement it via warnings it might be a wiser choice to make the new behaviour opt-in only.