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

Retain names of the 'repos' option #67

Merged
merged 3 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@jennybc
Copy link
Member

jennybc commented Jul 10, 2018

Fixes #66 Names of the repos option are being stripped

The test failure is the same that is already present on master FYI. updated: maybe all ok there

@jennybc jennybc requested a review from gaborcsardi Jul 10, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 10, 2018

Codecov Report

Merging #67 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   90.99%   91.02%   +0.02%     
==========================================
  Files          16       16              
  Lines         611      613       +2     
==========================================
+ Hits          556      558       +2     
  Misses         55       55
Impacted Files Coverage Δ
R/check.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a672290...87e6d22. Read the comment docs.

R/check.R Outdated
if (has("repos")) repos <- as.character(repos)
if (has("repos")) {
repos <- stats::setNames(as.character(repos), names(repos))
}

This comment has been minimized.

@jennybc

jennybc Jul 10, 2018

Author Member

This still means that all other attributes of getOption("repos") are lost (e.g. RStudio seems to set an attribute RStudio = TRUE). But I decided that was probably OK.

This comment has been minimized.

@gaborcsardi

gaborcsardi Jul 10, 2018

Member

Hmmm. I think we could maybe also just do

if  (has("repos"))  stopifnot(is.character(repos))

?

Or we could just not convert at all? After all, whatever you set as options("repos") should work in the other process.

This comment has been minimized.

@gaborcsardi

gaborcsardi Jul 10, 2018

Member

Yeah, I think we could just remove the conversions completely.

This comment has been minimized.

@jennybc

jennybc Jul 10, 2018

Author Member

OK. I was not really sure about the motivation for as.character() but went with it. I will make the change

@gaborcsardi gaborcsardi merged commit 3ecc197 into master Jul 10, 2018

1 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
@gaborcsardi

This comment has been minimized.

Copy link
Member

gaborcsardi commented Jul 10, 2018

Thanks much!

@jennybc jennybc deleted the retain-repos-names branch Sep 13, 2018

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