Allow turning off specific linters per line

[AshesITR]

Fixes #605

• implement exclusions in lint() and normalize_exclusions()
• implement exclusions in parse_exclusions()
• re-write old tests to new structure
• add new tests
• update internal documentation
• update documentation for exclude argument
• update documentation for comment based exclusions
• add tests for interleaved exclusions
• add vignette explaining in detail how to use exclusions

[AshesITR]

This is going to be a fairly large changeset.
I'd implement the linter exclusion stlye last suggested (and partly introduced into lintr itself already by @MichaelChirico)

That would mean the defaults work by appending :linter, linter2. to either of # nolint or # nolint start in order to exclude the named linters linter and linter2 for the marked line(s).

cc @jimhester @russHyde
Do you approve of this feature or have any feedback?

[russHyde]

Sorry so the syntax would be:

abc = 123 # nolint: assignment_linter

# nolint start: object_name_linter
MyClass <- function(x, ...) {
  # blah
}
# nolint end

Seems ok, is it obvious that the specified linters are being dropped?;
I note that pylint uses syntax of the formsome_code # pylint: disable=some_linter

[MichaelChirico]

What does the syntax look like for disabling optioned linters?

Mainly I have in mind something like

x <<- y # nolint: disable=undesirable_operator_linter(<<-) [??]

[AshesITR]

I'd actually always use the (unique) name that the user gives in .lintr.
For optioned linters that should also be the linter name by default.

@russHyde: almost. Note we added a full stop after the list, so it would be # nolint: assignment_linter.
I also took a look at what pylint does, but I feel this adds redundance to the comment, as we already say nolint.

The proposed syntax could be read as no lint (from) assignment_linter.

[AshesITR]

Here are some sample codes for easier side-by-side comparison of the two suggestions:

X = 42 # nolint: assignment_linter, object_name_linter. This is an explanation for the nolint

X = 42 # nolint: disable=assignment_linter,object_name_linter This is an explanation for the nolint

# nolint start: object_name_linter. This is an explanation for the nolint
MyClass <- function(x, ...) {
  # blah
}
# nolint end

# nolint start: disable=object_name_linter This is an explanation for the nolint
MyClass <- function(x, ...) {
  # blah
}
# nolint end

[AshesITR]

I think we need to solve #664 for usability with cusom linter settings. @MichaelChirico

[AshesITR]

I've added two new regexes to settings, defaulting to the : linter1, linter2, style.
They are called exclude_linter and exclude_linter_sep. I'm not really fond of the names, but can't find any better names at the moment. Suggestions are welcome.

The implementation part of this PR should be mostly done with the last commit.
Missing lots and lots of tests for the new behaviour and documentation.

Review on the implementation is welcome.

[MichaelChirico]

note that this means downstream custom linters can't have . in their name

[MichaelChirico]

I have this regex lying around btw for a valid R SYMBOL name:

[a-zA-Z][a-zA-Z0-9._]*|[.]|[.][a-zA-Z._][a-zA-Z0-9._]*

[AshesITR]

That complicates the : ... . syntax.
I'll open up a discussion whether we should still change the default here while it's hot.

I think using ( and ) as opener and closer respectively should turn out okay, but I'd like to make this change in a new PR, correcting all our internal # nolints for this as well.

WDYT?

[MichaelChirico]

@AshesITR sorry for the delay -- have been moving across the country since early December, finally getting settles with a good enough setup to do a review.

Some higher-level thoughts:

1. Another good follow-up would be the option to force exclusions to use a list, i.e. generic # nolint is not allowed. Specifying the linter makes "lint creep" far less likely, and makes it easier to search out all instances of a specific lint to boot.
2. The : <linters>. syntax looks a bit odd for nolint: start: <linters>. because the : is repeated. I still don't have a good alternative I'm happy with thought so not making it a blocker...
3. Somewhat related, some people might like to namespace-tag the linter exclusions, e.g. I keep my custom lints in a package myLinter and it makes it clearer to readers to see # nolint: lintr::seq_linter, myLinter::power_linter. Probably another follow-up issue to file.
4. Another power feature I think was mentioned at some point is non-overlapping range exclusions, e.g. 1:# nolint start: linterA ... 5:# nolint start: linterB 6: ... 7: # nolint end: linterA ... 12: # nolint end: linterB. I think the logic is partway there to supporting this, but it's not necessary to get this merged.

[AshesITR]

Thanks for you review, @MichaelChirico.
The GHA failure is due to the regex-based parsing of lint exclusions, i've tried fixing it by tricking with the \code command.

re 1.: crossref #690

re 2.: # nolint: start doesn't work by default, one might simply change the exclude_linter setting to a more pleasant regex as well.

The default is

^[[:space:]]*:[[:space:]]*(?<linters>(?:(?:[^,.])+[[:space:]]*,[[:space:]]*)*(?:[^,.])+)\.

What might work here (accepting # nolint: start (bla_linter), # nolint (bla_linter)) might be using (...) instead of :.. .

^[[:space:]]*\([[:space:]]*(?<linters>(?:(?:[^,.])+[[:space:]]*,[[:space:]]*)*(?:[^,.])+)\)

re 3.: The way we currently work is we take the name of the linter from the linters list. How does auto-naming work with custom linters? IINM the namespacing would be unnecessary. Not sure how to make this work with custom names either, imagine
list(linter_a = myPkg::a_linter(option = "a"), linter_b = myPkg::a_linter(option = "b"))

re 4.: This would be a breaking change once we publish, since it changes the semantics of e.g.

# nolint start: a.
# nolint start: b.
# nolint end
code_with_lint_type_a
# nolint end

Will throw a type a lint in the current implementation but won't if we add the "scoping" feature you propose.
Unless we would require # nolint end to be specific as well, in which case the new implementation should complain about a duplicate (unnecessary) # nolint end tag.

[MichaelChirico]

Unless we would require # nolint end to be specific as well

yes that's what i had in mind

[MichaelChirico]

I am happy to merge at this point. @russHyde and/or @jimhester it would be good to get your high-level input as well so we have a bit more consensus on the high-level "API" for this feature before release.

[AshesITR]

Let's merge for now and discuss the high-level API w.r.t. support for scoping and out-of-the-box support for . in linter names.

[bersbersbers]

I have spent some 20 minutes trying to figure out why

# nolint start: open_curly_linter

would also suppress other linters. In my eyes, the need to have a final dot even when a comment is not following makes it way too easy to do this wrong - especially since # nolint start: open_curly_linter does suppress the warning that you want to suppress. If you work from a baseline of few warnings, you will not even notice that you are suppressing all other future linters.

Something like #690 is definitely required if the dot requirement is not removed.