r-lib / lintr

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

Suggest a replacement in messages of `package_hooks_linter`? #2662

Open etiennebacher opened 1 week ago

etiennebacher commented 1 week ago

Some lintr messages show the potential replacement for a linter, e.g. in any_duplicated_linter:

library(lintr)
lint(
  text = "any(duplicated(x), na.rm = TRUE)",
  linters = any_duplicated_linter()
)
#> <text>:1:1: warning: [any_duplicated_linter] anyDuplicated(x, ...) > 0 is better than any(duplicated(x), ...).
#> any(duplicated(x), na.rm = TRUE)
#> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

However, some messages are not very helpful because they don’t recommend any replacement. For example, package_hooks_linter says Don't use cat() in .onAttach().:

lint(
  text = ".onLoad <- function(lib, pkg) cat('hi')",
  linters = package_hooks_linter()
)
#> <text>:1:31: warning: [package_hooks_linter] Don't use cat() in .onLoad().
#> .onLoad <- function(lib, pkg) cat('hi')
#>                               ^~~

Is it possible to suggest the replacement for this linter? My reflex was to use message() instead but this is also flagged. Same with packageStartupMessage() (only flagged in .onLoad(), not in .onAttach()) so I don’t know what should be used here.

MichaelChirico commented 1 week ago

messages should be produced by .onAttach, not .onLoad

etiennebacher commented 1 week ago

Then I think messages from .onLoad should say something like:

Don't use cat() in .onLoad(). Use packageStartupMessage() in .onAttach() instead.

and messages from .onAttach() could say:

Don't use cat() in .onAttach(). Use packageStartupMessage() instead.

AshesITR commented 1 week ago

I suggest using a more active lint message for .onAttach:

Use packageStartupMessage() instead of cat() in .onAttach().