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

New match_symbol_usage argument to undesirable_function_linter #814

Closed
MichaelChirico opened this issue Jun 24, 2021 · 3 comments · Fixed by #844
Closed

New match_symbol_usage argument to undesirable_function_linter #814

MichaelChirico opened this issue Jun 24, 2021 · 3 comments · Fixed by #844

Comments

@MichaelChirico
Copy link
Collaborator

We have a function foo() that we'd like to lint. However, foo() comes from the package foo, which is still OK, so we have the following unfortunate situation:

library(foo) # triggers a lint, but shouldn't
foo::foo()  # triggers a lint, as it should

The issue is that undesirable_function_linter by default also looks for SYMBOL usage of input functions, which is good for catching usage within sapply, purrr::map, etc.

So we can add a new argument to toggle whether SYMBOL is included, and run linting with

with_defaults(
  undesirable_function_linter(c("return")), # match_symbol_usage = TRUE by default
  undesirable_function_linter(c("foo"), match_symbol_usage = FALSE)
)

Happy to file the PR if that sounds like a good idea.

@MichaelChirico
Copy link
Collaborator Author

alternatively we could treat this as a bug since it basically is a false positive. solution would be to switch to XML search in the linter & exclude parent library/require calls...

@kpagacz
Copy link
Contributor

kpagacz commented Jul 15, 2021

I personally would treat it as a false positive.

Do you mind if I started working on this?

@MichaelChirico
Copy link
Collaborator Author

sure! PR is a good place to continue the discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants