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 argument minimum_version to skip_if_not_installed() #499

Closed
wants to merge 9 commits into from

Conversation

@krlmlr
Copy link
Member

@krlmlr krlmlr commented Jul 4, 2016

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.

Kirill Müller added 3 commits Jul 4, 2016
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(

This comment has been minimized.

@hadley

hadley Jul 5, 2016
Member

I'd rather keep the existing check - this feels a bit too clever to me.

This comment has been minimized.

@krlmlr

krlmlr Jul 5, 2016
Author Member

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

This comment has been minimized.

@krlmlr

krlmlr Jul 5, 2016
Author Member

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.

This comment has been minimized.

@hadley

hadley Jul 5, 2016
Member

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 krlmlr force-pushed the krlmlr:feature/#487-skip-min-version branch from 566b04e to e10f3e2 Jul 5, 2016
@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Jul 5, 2016

Reinstalled the old check, but still need to test version number manually.

@krlmlr
Copy link
Member Author

@krlmlr krlmlr commented Jul 6, 2016

Now requiring package first, then checking version if appropriate.

@hadley
Copy link
Member

@hadley hadley commented Dec 15, 2016

LGTM. I'll merge when I'm next at my computer.

@hadley hadley closed this in 0312fc7 Dec 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants