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

add 'remotes.upgrade' option for upgrade toggle #240

Merged
merged 1 commit into from Nov 15, 2018

Conversation

kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Nov 7, 2018

This allows users to specify a default upgrade preference (for those of us who might prefer automatically updating dependent packages when installing the development version of a package).

@gaborcsardi
Copy link
Contributor

gaborcsardi commented Nov 7, 2018

Thanks! Sounds good. However, we already have some environment variables, see https://github.com/r-lib/remotes#environment-variables

Environment variables are better in some ways, e.g. subprocesses inherit them, and you can set them globally for all R sessions.

Do you have a particular reason to have an option instead of an env var?

@kevinushey
Copy link
Contributor Author

kevinushey commented Nov 7, 2018

Using an R option just seemed the most natural mechanism when I first thought about it, but I'd be entirely open to using environment variables (especially if we want / need to think about subprocesses). Would using e.g. Sys.getenv("REMOTES_UPGRADE", "ask") be better?

@gaborcsardi
Copy link
Contributor

gaborcsardi commented Nov 7, 2018

Would using e.g. Sys.getenv("REMOTES_UPGRADE", "ask") be better?

Yeah, I like that. Can you please also add it to the list of env vars in the README? Thanks!

@kevinushey kevinushey force-pushed the feature/install-ask-option branch from 240a495 to 3d7a799 Compare Nov 7, 2018
R/deps.R Outdated
@@ -529,6 +529,14 @@ remote_deps <- function(pkg, ...) {
res
}

upgrade_default <- function() {

env <- Sys.getenv("R_REMOTES_UPGRADE", unset = NA)
Copy link
Member

@jimhester jimhester Nov 8, 2018

Choose a reason for hiding this comment

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

Couldn't this whole function just be

Suggested change
env <- Sys.getenv("R_REMOTES_UPGRADE", unset = NA)
Sys.getenv("R_REMOTES_UPGRADE", "ask")

@jimhester
Copy link
Member

jimhester commented Nov 8, 2018

My main concern with this is you lose the auto-documenting behavior of c("ask", "always", "never").

An alternative would be to keep the arguments the same and add logic to

https://github.com/jimhester/remotes/blob/8dd72c74ae34e04851f34f5b7b33329572107a43/R/deps.R#L533-L547

We are already changing the default for non-interactive sessions, so maybe just adding it there is ok.

@gaborcsardi
Copy link
Contributor

gaborcsardi commented Nov 8, 2018

We could have upgrade = c("default", "ask", "always", "never"), and then "default" would do Sys.getenv("R_REMOTES_UPGRADE", if (interactive()) "ask" else "always").

@kevinushey kevinushey force-pushed the feature/install-ask-option branch from 3d7a799 to 70959f2 Compare Nov 15, 2018
@kevinushey
Copy link
Contributor Author

kevinushey commented Nov 15, 2018

I've updated the PR with the new "default" option -- let me know how it looks.

@jimhester
Copy link
Member

jimhester commented Nov 15, 2018

This LGTM

@gaborcsardi gaborcsardi merged commit bd970d7 into r-lib:master Nov 15, 2018
4 checks passed
@gaborcsardi
Copy link
Contributor

gaborcsardi commented Nov 15, 2018

Thanks!

gaborcsardi added a commit that referenced this issue Nov 17, 2018
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