linter exclusions bug

[russHyde]

Running lint_package(linters = object_name_linter()) on current master of lintr:

R/utils.R:224:1: style: Variable and function name style should be snake_case or symbols.
Linter <- function(fun) { # nolint: object_name_linter.
^~~~~~

and a couple of similar issues.

Note that if the nolint comment for Linter in R/utils.R is changed, removing the terminal dot:

# R/utils.R
Linter <- function(fun) { # nolint: object_name_linter
  # blah
}

... no lint is thrown for the name Linter

[russHyde]

Note further, if the call to lint_package is made with a named list (and with the lint-exclusion comments as they initially were, with the closing dot), no object-name lints are thrown:

lint_package(linters = list(object_name_linter = object_name_linter()))

(but now, if I remove the terminal dot in the exclusion comment, the object-name lints are thrown)

[russHyde]

The expected behaviour here is:

# R/file.R
Some_class <- function(x) { # nolint object_name_linter.
}
Another_class <- function(x) { # nolint object_name_linter
}

lint_package(linters = object_name_linter()) # Some_class is green; Another_class is red (or possibly a warning for misspecified linter-exclusion syntax)

lint_package(linters = list(object_name_linter = object_name_linter())) # Same results as the previous command

[AshesITR]

At first glance this looks like a regression introduced by the Linter class PR.
Could you check names(linters) in the MWE?

Re expected behaviour: I'd expect Another_class to be green as well since failure to parse exclusions should cause a blanket # nolint behaviour, suppressing all lints.

[AshesITR]

Here's the problem:

>names(linters)
[1] "object_name_linter()"

We need to change the implementation of auto_names().

[AshesITR]

@russHyde WDYT about the defaults being the match of "^[A-Za-z._]+" against deparse(...) and then passing defaults to make.unique() to ensure uniqueness?

This would auto name list(line_length_linter(120), line_length_linter(80)) as c("line_length_linter", "line_length_linter_1") if we make.unique(..., sep = "_")

The changed separator is necessary if we stick with the current specification for linter exclusions as it does not allow dots in the names.

What I don't like about make.unique is that it only renames the second duplicate.

[AshesITR]

I think we have an even bigger problem. With the custom linters going in, deparse returns basically the source code of the Linter.
This could be fixed by adding a name attribute to the Linter class and making Linter() smart about auto-naming. WDYT?

[russHyde]

Do you think anyone would be using multiple copies of the same Linter in their set of linters?

I'm afraid I didn't follow the bit about deparse'ing a custom linter.

Making the Linter class smarter about naming would clarify the code a bit. For example, there's duplication of the linter-name in a few places in the code-base and it can be a bit confusing

# the user might call
lint(".", linters = list(some_linter = some_linter()))

# the "linter" in the output is the same as the function-name
some_linter <- function(...) {
   ...
   Lint(..., linter = "some_linter")
}

[AshesITR]

I've started a draft to make this smarter.
Re multiple linters of the same type: line_length_linter is a bad example, but a more complete example for shiny apps to force all library() statements to occur at a specific location:

linters = with_defaults(undesirable_function_linter(default_undesirable_functions[-4]), library_linter = undesirable_function_linter(list(library = "place all library calls at the top of 'global.R'"))

Could work nicely with

global.R

# nolint start: library_linter.
# place all library calls here
library(shiny)
# ...
# nolint end