Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest a replacement in messages of package_hooks_linter? #2662

Open
etiennebacher opened this issue Sep 7, 2024 · 3 comments
Open

Suggest a replacement in messages of package_hooks_linter? #2662

etiennebacher opened this issue Sep 7, 2024 · 3 comments
Labels
feature a feature request or enhancement

Comments

@etiennebacher
Copy link
Contributor

etiennebacher commented Sep 7, 2024

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.

@etiennebacher etiennebacher changed the title Suggest a replacement in package_hooks_linter? Suggest a replacement in messages of package_hooks_linter? Sep 7, 2024
@MichaelChirico
Copy link
Collaborator

messages should be produced by .onAttach, not .onLoad

@etiennebacher
Copy link
Contributor Author

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
Copy link
Collaborator

AshesITR commented Sep 7, 2024

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

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

@IndrajeetPatil IndrajeetPatil added the feature a feature request or enhancement label Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants