Skip to content

Set metadata in packrat lockfile#429

Merged
kevinushey merged 5 commits into
rstudio:masterfrom
cderv:set-metadata
Dec 4, 2017
Merged

Set metadata in packrat lockfile#429
kevinushey merged 5 commits into
rstudio:masterfrom
cderv:set-metadata

Conversation

@cderv

@cderv cderv commented Dec 1, 2017

Copy link
Copy Markdown
Contributor

Hey,

This is related #425. @kevinushey I followed you advice and made one function to change different metadata. Just two for the moment repos and r_version. I think the others are not useful to modify.

Can you take a look and tell me if the direction I took is ok ?

I tried to write some tests but I don't manage to make them work. It seems like other test are not working either.
I tried something like that

projRoot <- cloneTestProject("healthy")
  init(enter = FALSE, projRoot, options = list(local.repos = "packages"))

but init does not work as expected.
help appreciated for those tests. thanks.


This change is Reviewable

Comment thread R/lockfile-metadata.R
#' # Setting back old state
#' # set_lockfile_metadata(r_version = old_rver)
#' }
set_lockfile_metadata <- function(repos = NULL, r_version = NULL, project = NULL) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Most of the Packrat APIs take the project as the first argument -- can we follow that convention here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I followed the convention of set_opts and get_opts that I find close in functionality. project is last in get_opts and second in set_opts. From usage :

get_opts(options = NULL, simplify = TRUE, project = NULL)
set_opts(..., project = NULL, persist = TRUE)

bad idea ?
Still want me to change ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good point! I think you're right; let's leave it as is then.

Comment thread R/lockfile-metadata.R Outdated
stop(paste(lockFilePath, " is missing. Run packrat::init('",
project, "') to generate it.", sep = ""))
}
lf <- as.data.frame(readDcf(lf_filepath), stringsAsFactors = F)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we use FALSE instead of the abbreviated F here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Absolutely right ! Good practice still not automatic for me. Done!

@kevinushey kevinushey merged commit 9cab3f1 into rstudio:master Dec 4, 2017
@kevinushey

Copy link
Copy Markdown
Contributor

Thanks!

@cderv

cderv commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

Oh thanks for merging!
I thought it needed a test file but as I did not manage to make it work I did not push it. If you want me to still try for later do not hesitate to tell me!
Should we add a bullet to NEWS.md too ?
I also closed manually the related issue. Forgot to add closing keyword in the PR or commit message.

thanks again for you review !

@kevinushey

Copy link
Copy Markdown
Contributor

I added a NEWS bullet and bumped the version here:

6ff1185

I'd welcome a PR adding tests as well if you have the time to put something together as well. Thanks for taking the time to put this together!

@cderv

cderv commented Dec 4, 2017

Copy link
Copy Markdown
Contributor Author

Thanks, should have looked in the other commit..
For the test, I tried to get inspiration from other tests but they are skipped on cran and does not seem to work anymore. I did not manage to created a dummy packrat project in temp folder.i will keep trying and I will come up with something eventually. You will welcome a PR when it is ready. No rush but will be better if tested.

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.

2 participants