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

added support for pacman::p_load package loaders #360

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xhdong-umd
Copy link

@xhdong-umd xhdong-umd commented Feb 23, 2017

Background: issue in rsconnect and pacman

The issue is packrat doesn't recognize pacman package loader calls.

First I tried the methods used in original code for checking library, require, but the match.call method doesn't work with pacman::p_load because the package name argument is not named.

Then I just scan the arguments in the pacman loader call, excludes all other arguments that are not package names, then add the package names to the environment.

I tested the code in file level packrat:::fileDependencies(file) which seemed be able to recognize pacman loader correctly. However when I tried to deploy my shiny apps with pacman loader using my custom built packrat, there is error

Error: Unable to retrieve package records for the following packages:

  • 'packrat'

I'm not sure how to fix this error since I'm just building and loading my version of packrat, which could cause some problems. I'm submitting the pull request for review first.

If there is some method to avoid the error in shiny deployment, I'll test more with shiny deployment with my code.

Known limitations: p_load_gh is not implemented because the github repo string could be quite complicated. Instead user just need to load that package in p_load again, which will make sure the packages be picked up by packrat, and there is no negative impact of this.

@xhdong-umd
Copy link
Author

I fixed the problem in deploying by installing packrat from my forked github repo. I tested with my app that used pacman loader and everything worked.

I also created a simple test file to test the changes. There are some known limitations:

supported syntax

pacman::p_load(stringr, units, install = FALSE)
pacman::p_load(shiny, shinydashboard)

unsupported syntax

  • arguments that saved in variable.
    op <- TRUE
    pacman::p_load(shinyjs, install = op)
    pkgs <- c("dplyr", "tibble")
    pacman::p_load(pkgs, character.only = TRUE)
  • similar usage for library cannot be detected with original packrat either. I assume this kind of usage is rare and that means some more dynamical package loading, which is not suitable for shiny deployment.
    pkg <- "ggmap"
    library(pkg, character.only = TRUE)

note pacman package itself will be picked up by original code no matter what:
if used pcaman::p_load, will pick up because of ::
if used library(pacman), will pick up because of library.

@xhdong-umd
Copy link
Author

xhdong-umd commented Feb 24, 2017

I added a Rmarkdown to explain more details, and a test file I used to test the code. The test file is just a script with different pacman syntax. Proper testthat test case can be added later if needed.

This test file caused some error in CI checks. However the test will need a file (cannot run with a string), and I'm not sure how to avoid this error unless I put the file elsewhere.

@dracodoc
Copy link

Hi @jcheng5 do you have time to review the PR?

@kevinushey
Copy link
Contributor

Thanks for taking the time to create this PR. However, I'd prefer to avoid adding this directly to Packrat (which would imply we'd have to maintain + update it), and instead provide a mechanism for users to register their own dependency resolution routines.

@xhdong-umd
Copy link
Author

xhdong-umd commented Mar 16, 2017

If rsconnect can provide such a mechanism that will be great. It's easy to just use some configuration file format like dcf or yaml to register dependencies, following existing convention in DESCRIPTION of package development.

Then I can suggest to add this feature to packman and create proper dependency config file, let rsconnect or packrat read it.

@stanstrup
Copy link

stanstrup commented Sep 22, 2017

Would be very nice to get this merged or otherwise made possible...

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.

4 participants