-
-
Notifications
You must be signed in to change notification settings - Fork 51
Update lintr hook and snippet_generate for lintr hook #650
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi @BjarkeHautop, thanks for your contributions. Before you spend time on code changes, let's discuss them in an issue as per the CONTRIBUTING.md. The |
|
I originally mentioned the motivation here: #648 (I realize that isn’t an issue). For {styler}, things work because the config excludes all the locations where it would otherwise fail, so it doesn't actually run on the whole package: My concern is that a user sees the package, tries the default hooks on their own (non-trivial) package, and lintr-hook will fail with no clear way to address it. So, the user now needs to look at github issues and/or go through documentation to find out why (in particular that happend to me at #647), or they may simply decide not to use the package at all. That said, the part of the PR in commit 1aa1ff5 (possibly with a small modification) still addresses the issue that adding only hard dependencies via |
|
Sorry I don’t care if issue or pull request, but I meant that I never endorsed the idea of a change in the default before you started to implement it. I see what you are saying. Another way would be to only include Also, I am happy to accept changes with regard to snippet generation anyways, as long as the default is not changed. |
|
No, that won't fix the issue. See e.g. the following two simple examples, which would fail lintr-hook currently (first example equivalent to #647): Fails lintr-hook because it uses @importFrom pkg without loading the pkg. Another example of it failing is trying to use a function defined in a different R file. So have this in R/plus_one.R And this in R/plus_two.R: It will also fail the lintr-hook. I guess a possible solution is to avoid doing the object-usage check from lintr if one doesn't want to load the package. So, add this to .lintr |
This changes the
snippet_generate('additional-deps-lintr')to also include suggested dependencies, so {lintr} works on vignettes, README, etc. Modifed default hook to includeargs: [--load_package]for lintr.These changes prevents the default hooks from automatically failing when calling
use_precommit()on a package, since {lintr} would fail if the package had any dependencies (or simply when the package calls its own function from another file).Also removed an old print about roxygenize that always appeared.