New argument minimum_version to skip_if_not_installed() #499
Conversation
instead of requireNamespace(); the package isn't loaded anymore as a side effect, and the check also works if the package is loaded already
| if (requireNamespace(pkg, quietly = TRUE)) { | ||
| return(invisible(TRUE)) | ||
| skip_if_not_installed <- function(pkg, minimum_version = NULL) { | ||
| tryCatch( |
There was a problem hiding this comment.
I'd rather keep the existing check - this feels a bit too clever to me.
There was a problem hiding this comment.
I don't like the existing check for its side effect, it causes the package to be loaded. Furthermore, requireNamespace() doesn't check the version once the package is loaded. I could move the skip() call outside the tryCatch() if that helps
There was a problem hiding this comment.
Or I could call requireNamespace() in addition to the manual version check -- this would work even if the installed package is somehow damaged so that DESCRIPTION exists but the package cannot be loaded.
There was a problem hiding this comment.
Loading doesn't seem like a bad side effect to me - you're assuming checking it's present before you attempt to use it to do something.
krlmlr
force-pushed
the
feature/#487-skip-min-version
branch
from
566b04e
to
e10f3e2
Compare
|
Reinstalled the old check, but still need to test version number manually. |
|
Now requiring package first, then checking version if appropriate. |
|
LGTM. I'll merge when I'm next at my computer. |
The package isn't loaded anymore as a side effect, and the check also works if the package is loaded already. With test.
Closes #487.