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

Ramd::packages can install from GitHub #8

Merged
merged 22 commits into from
Jan 20, 2016
Merged

Conversation

peterhurford
Copy link
Collaborator

No description provided.

@robertzk
Copy link
Owner

Can you re-run document?

if (is_github_package(name)) {
name <- strsplit("robertzk/Ramd", "/")[[1]][[2]]
}
require(name, character.only = TRUE)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check in utils::installed.packages instead? A function with this name shouldn't be actually loading the package into memory.

@kirillseva
Copy link
Collaborator

nice!

#' @examples
#' \dontrun{
#' load_package('glmnet')
#" load_package("glmnet")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong quote on the left hand side

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oops 😮

if (!require(name, character.only = TRUE))
stop(paste('Package', name, 'not found'))
load_package <- function(name, verbose = FALSE) {
if (is.list(name)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is too long now. Can you split it up?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace this whole section with:

metadata <- name[-1]
name <- name[[1]]

and then below replace

if (!is.null(metadata)) {
  do.call(devtools::install_github, c(name, metadata))
}

with

if (length(metadata)) {
  do.call(devtools::install_github, c(list(name), metadata))
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

# extract 0.3 from robertzk/Ramd@v0.3
name <- strsplit(name, "@")[[1]][[2]]
if (grepl("v", name, fixed = TRUE)) {
name <- strsplit(name, "v")[[1]][[2]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the author name has a v? This is terrible. You should use @v at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch... and unnecessary.

It is only separating everything after the @, so it would not include the author. Added some tests to prove that.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see.


if (is_github_package(name)) {
remote <- "GitHub"
install_from_github(name, metadata, remote, verbose)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the remote parameter? That should be hardcoded into the install_from_github method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemed not DRY to me to duplicate it both here and in the method itself. You need remote outside to work for package not found on line 35.

Another option is to include the sub-functions within the load_package function so they can access remote as a global (without needing to be passed as a parameter), but I don't really like that as a style and it will lead to triply nested functions.

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm fine with this.

@robertzk
Copy link
Owner

Thanks!

robertzk added a commit that referenced this pull request Jan 20, 2016
Ramd::packages can install from GitHub
@robertzk robertzk merged commit 1ef26c0 into master Jan 20, 2016
@robertzk robertzk deleted the packages_from_github branch January 20, 2016 19:52
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.

None yet

3 participants