lintr icon indicating copy to clipboard operation
lintr copied to clipboard

Investigate further warning message seen in lint GHA workflow

Open IndrajeetPatil opened this issue 2 years ago • 1 comments

cf. https://github.com/r-lib/lintr/actions/runs/3240547631/jobs/5311303504

Warning message:
In add_exclusions(exclusions, excluded_lines, linters_string, exclude_linter_sep,  :
  Could not find linter named ‘one_linter’ in the list of active linters. Make sure the linter is uniquely identified by the given name or prefix.

IndrajeetPatil avatar Oct 13 '22 16:10 IndrajeetPatil

It's coming from here, not sure why it's only happening in that workflow though:

https://github.com/r-lib/lintr/blob/4b37029f989629287e910fa3aba46ab15d1975cb/tests/testthat/test-unreachable_code_linter.R#L124-L138

MichaelChirico avatar Oct 13 '22 17:10 MichaelChirico

There is a new warning now, in addition to the existing one:

1: In parse_check_usage(fun, known_used_symbols = known_used_symbols,  :
  Possible bug in lintr: Couldn't parse usage message ‘<anonymous> : <anonymous> : <anonymous>: local variable 'col2' assigned but may not be used
’. Ignoring 2 usage warnings. Please report an issue at https://github.com/r-lib/lintr/issues.

IndrajeetPatil avatar Oct 23 '22 06:10 IndrajeetPatil

cf. https://github.com/r-lib/lintr/actions/runs/3240547631/jobs/5311303504


Warning message:

In add_exclusions(exclusions, excluded_lines, linters_string, exclude_linter_sep,  :

  Could not find linter named ‘one_linter’ in the list of active linters. Make sure the linter is uniquely identified by the given name or prefix.

That should easily be fixed by changing the nolint directive like we do in other tests.

AshesITR avatar Oct 23 '22 16:10 AshesITR

There is a new warning now, in addition to the existing one:


1: In parse_check_usage(fun, known_used_symbols = known_used_symbols,  :

  Possible bug in lintr: Couldn't parse usage message ‘<anonymous> : <anonymous> : <anonymous>: local variable 'col2' assigned but may not be used

’. Ignoring 2 usage warnings. Please report an issue at https://github.com/r-lib/lintr/issues.

Those were previously silently ignored I presume. I might try running with options(warn = 3) later to find the source.

AshesITR avatar Oct 23 '22 16:10 AshesITR

Surprised that these weren't caught here, but maybe they are not coming from the /tests folder:

https://github.com/r-lib/lintr/blob/1ac48e94ef550945d94c0e47b43a9eb8041b5f00/.github/workflows/test-package-vigilant.yaml#L30

IndrajeetPatil avatar Oct 24 '22 09:10 IndrajeetPatil

They are emitted by lint_package(), not by test(). That's why. WDYT about using options(warn = 2) in the linting workflows as well? lint() isn't supposed to produce warnings if everything is in order, so that might be a good way to catch these early.

AshesITR avatar Oct 29 '22 12:10 AshesITR

I have an update on the check usage problem:

single_quotes_linter.R houses the issue, but this looks like a bug in codetools triggered by our use of assignments within with():

codetools::checkUsage(function() {
  with(
    list(),
    a <- 1
  )
}, skipWith = TRUE)
#> <anonymous>: local variable 'a' assigned but may not be used
codetools::checkUsage(function() {
  with(
    list(),
    a <- 1
  )
}, skipWith = FALSE)
#> <anonymous>: local variable 'a' assigned but may not be used (<text>:8-11)

Created on 2022-11-09 with reprex v2.0.2

AshesITR avatar Nov 09 '22 21:11 AshesITR

Thanks for tracking down the source of this! Should this be reported in r-devel mailing list?

As a temp workaround, we could avoid assignments. That is, change the following

https://github.com/r-lib/lintr/blob/c814159f98eee8004319a027e1a24b7887455e9f/R/single_quotes_linter.R#L52-L64

to

        with(content[str_idx[id], ], {
          Lint(
            filename = source_expression$filename,
            line_number = line1,
            column_number = col1,
            type = "style",
            message = "Only use double-quotes.",
            line = source_expression$file_lines[[line1]],
            ranges = list(c(col1,  if (line1 == line2) col2 else nchar(line)))
          )
        })

IndrajeetPatil avatar Dec 05 '22 11:12 IndrajeetPatil

Not a big fan of the refactoring, as it makes the code harder to read. Maybe instead write col1 <- content$col1[str_idx[id]] etc. for all the variables from content? That would allow us to drop the with() alltogether.

AshesITR avatar Dec 05 '22 12:12 AshesITR

I also see this warning locally now if I run (e.g.) the following code:

> lintr::lint_package(linters = lintr::commented_code_linter())
  |================================================================================================| 100%
Warning message:
In add_exclusions(exclusions, excluded_lines, linters_string, exclude_linter_sep,  :
  Could not find linter named ‘one_linter’ in the list of active linters. Make sure the linter is uniquely identified by the given name or prefix.

IndrajeetPatil avatar Sep 12 '23 07:09 IndrajeetPatil

It's thrown by the same file, and not by executing the test, because the test suite defines one_linter().

This is a general problem with how # nolint: specific_linter. works. I think we can safely ignore the warning; an alternative is to refactor that test to use a linter actually in our test suite.

I believe we've discussed alternatives for this warning before but not really gotten anywhere so far.

MichaelChirico avatar Sep 12 '23 21:09 MichaelChirico

cf. https://github.com/r-lib/lintr/actions/runs/3240547631/jobs/5311303504


Warning message:

In add_exclusions(exclusions, excluded_lines, linters_string, exclude_linter_sep,  :

  Could not find linter named ‘one_linter’ in the list of active linters. Make sure the linter is uniquely identified by the given name or prefix.

That should easily be fixed by changing the nolint directive like we do in other tests.

Using # TestNolint: one_linter. or whatever other nolint directive seems to be a good way to solve the issue IMO.

AshesITR avatar Sep 13 '23 20:09 AshesITR