Skip to content

Fix warning in p_install_gh when installing multiple packages#122

Open
LoHertel wants to merge 1 commit intotrinker:masterfrom
LoHertel:fix-install-gh
Open

Fix warning in p_install_gh when installing multiple packages#122
LoHertel wants to merge 1 commit intotrinker:masterfrom
LoHertel:fix-install-gh

Conversation

@LoHertel
Copy link

This PR addresses the warning message mentioned in #118 when installing multiple packages with p_install_gh().

The code in R/p_install_gh.R#L24 throws a warning message when passing a vector of repository names to the package argument: p_install_gh(c('path/package', 'path2/repo'))

Warning message:
if (p_loaded(char = package)):
  the condition has length > 1 and only the first element will be used

I propose the following changes:

  • If clause should check for an array of booleans instead of a single boolean.
  • Because p_loaded() is using the package name to check, whether that package is loaded, the package name should be extracted from the Github path beforehand with parse_git_repo(). (Because p_install_gh() passes a Github path (e.g. 'trinker/pacman') to p_loaded(). p_loaded() always returns FALSE for Github paths.)
  • Update the code of parse_git_repo() in R/utils.R#L194 from the remotes package. It would act as an interface to call either remotes::parse_github_url() or remotes::parse_repo_spec(). The code for parse_git_repo() is taken from remotes/R/parse-git.R#L97. parse_git_repo() depends on viapply(), therefore it is included from remotes/R/utils.R#L8 as well.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 44ceb33 on LoHertel:fix-install-gh into fc09ccd on trinker:master.

@LoHertel LoHertel changed the title Fix that p_install_gh installes multiple packages without warning Fix warning in p_install_gh when installing multiple packages May 22, 2019
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 this pull request may close these issues.

2 participants