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

Clean up install_github parameters pull and branch #509

Merged
merged 8 commits into from Jun 30, 2014

Conversation

Projects
None yet
2 participants
@krlmlr
Member

krlmlr commented Jun 26, 2014

The ref and pull parameters are mutually exclusive, and the branch parameter has been deprecated. The latter two should be removed, and the ref parameter should now also accept functions.

If ref is a function, it will be called with a named list that contains all other parameters (after parsing with github_parse_path and obtaining access tokens), and return an updated version of this list. Implementation and usage examples:

github_pull_request <- function(pull) {
  force(pull)
  function(param) {
    pullinfo <- github_pull_info(param$repo, param$username, param$pull)
    param$username <- pullinfo$username
    param$ref <- pullinfo$ref
    param
  }
}

install_github("devtools", ref = github_pull_request(475))

github_pull_request <- function(pull) {
  force(pull)
  function(param) {
    pullinfo <- github_pull_info(param$repo, param$username, param$pull)
    param$username <- pullinfo$username
    param$ref <- pullinfo$ref
    param
  }
}

github_release <- function(pos = 1, pre_release = TRUE) {
  force(pos)
  force(pre_release)
  function(param) {
    param$ref <- ...
    param
  }
}

install_github("devtools", github_release())

This would allow for a cleaner implementation of #350.

Usage of the deprecated parameters pull and branch can be detected by looking at ....

Thoughts?

@hadley

This comment has been minimized.

Member

hadley commented Jun 26, 2014

I'd prefer to use some simple S3 dispatch.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jun 26, 2014

Do you mean something like this:

github_ref <- function(x) UseMethod("github_ref")
github_ref.default <- function(x) list(ref=x)
github_ref.pull <- function(x) {
  list(repo=xxx, username=yyy, ref=zzz)
}
github_pull <- function(pull) structure(pull, class="pull")

# in github_get_conn:
ref_params <- github_ref(ref)
params[names(ref_params)] <- ref_params

?

@hadley

This comment has been minimized.

Member

hadley commented Jun 26, 2014

Yes, exactly, except I'd write the last bit as ref_params <- modifyList(params, github_ref(ref))

Kirill Müller added some commits Jun 26, 2014

Kirill Müller
Deprecate and remove pull parameter to install_github
- New exported function github_pull() to be used as value for ref parameter
- Implemented using a simple S3 dispatch mechanism
- Tests with improved mocking
    - new function with_mock
- Tests for warning when using deprecated parameters branch and pull
@krlmlr

This comment has been minimized.

Member

krlmlr commented Jun 26, 2014

This implementation seems to work for me.

@@ -69,26 +67,18 @@ github_get_conn <- function(repo, username = getOption("github.user"),
ref <- branch
}
if (!is.null(pull)) {

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Won't this always be false?

This comment has been minimized.

@krlmlr

krlmlr Jun 27, 2014

Member

Not if an explicit pull = xxx is passed to install_github -- it will be captured by ... and assigned to pull in the call to install_github_single.

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Oh I see - missed the separation of the functions in the diff.

param <- modifyList(param, github_ref(param$ref, param))
param$msg <- with(

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Can you please not use with()? I prefer to avoid non-standard evaluation in functions (plus it makes it harder to understand where the variables are coming from)

param$msg <- with(
param,
paste0("Installing github repo ",
paste(repo, ref, sep = "/", collapse = ", "),

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Just two spaces indent please

@@ -172,6 +170,24 @@ install_github_single <- function(repo, username = getOption("github.user"),
config = conn$auth, before_install = github_before_install, ...)
}
github_ref <- function(x, param) UseMethod("github_ref")
github_ref.default <- function(x, param) list(ref=x)

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

You need to @export the methods

This comment has been minimized.

@krlmlr

krlmlr Jun 27, 2014

Member

They are not intended to be called directly -- the user is not expected to ever call github_ref(). Do we still need to @export?

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Yes - @export exports the S3 method definitions, which you must export (otherwise the generic won't find them)

This comment has been minimized.

@krlmlr

krlmlr Jun 27, 2014

Member

This conflicts with my perception:

  • The tests succeed, both in devtools::test and R CMD check
  • install_github with ref = github_pull(xxx) works correctly when called from a fresh R session

What have I overlooked?

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

What if you don't attach devtools and do devtool::install_github("devtools", ref = devtools::github_pull(509)) ? I think that might be the problematic case. I'm pretty sure not exporting S3 method has bitten me in the past.

This comment has been minimized.

@krlmlr

krlmlr Jun 27, 2014

Member

I have tested the example you provided in a fresh session without devtools attached, it runs without error. It is true that getS3method("github_ref") doesn't find anything, with or without devtools attached. I have also noticed that the following fails:

devtools:::github_ref(devtools:::github_pull(509))

Both with and without devtools attached.

But I think this only matters if we want the user to call github_ref(), which we don't. Only devtools code is supposed to ever call github_ref(). (I have also tested creating the generic inside the function github_get_conn -- works as well!)

Since the tests succeed and the implementation works -- may I suggest abstaining from exporting for the time being? Adding an export can be done easily, but un-exporting something hardly ever happens. Also, this would save us from adding documentation for an otherwise useless item.

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

I would honestly prefer to export every S3 method, regardless of whether or not the generic is exported. Then I don't need to think about it.

github_ref <- function(x, param) UseMethod("github_ref")
github_ref.default <- function(x, param) list(ref=x)
github_ref.pull <- function(x, param) {
#param <-

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

Remove commented code?

@@ -228,6 +244,15 @@ github_parse_path <- function(path) {
ret[sapply(ret, nchar) > 0]
}
github_assign_ref <- function(params) {

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

I don't understand the point of this function. Why would pull not be null?

This comment has been minimized.

@hadley

hadley Jun 27, 2014

Member

I think this code should be moved into github_parse_path() - i.e. it should return a pull_request object in the ref field.

@krlmlr

This comment has been minimized.

Member

krlmlr commented Jun 27, 2014

Should be okay now.

hadley added a commit that referenced this pull request Jun 30, 2014

Merge pull request #509 from krlmlr/509-install_github-s3-ref
Clean up install_github parameters pull and branch

@hadley hadley merged commit 68d37d0 into r-lib:master Jun 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Jun 30, 2014

Thanks!

@krlmlr krlmlr deleted the krlmlr:509-install_github-s3-ref branch Jun 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment