-
Notifications
You must be signed in to change notification settings - Fork 64
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
New force_identical_write
arg
#735
Changes from 4 commits
b30eadd
5180df4
d313fbe
312171c
04565eb
28f0ecd
856b26c
4f219ad
7966273
01508aa
d93dab9
7503901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,10 @@ pin_read <- function(board, name, version = NULL, hash = NULL, ...) { | |
#' use the default for `board` | ||
#' @param tags A character vector of tags for the pin; most important for | ||
#' discoverability on shared boards. | ||
#' @param check_hash Check whether the pin contents are identical to the last | ||
#' version if one exists (using the hash), and then **do not store** the pin | ||
#' again. This argument does not check the pin metadata, only the pin | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The wording isn't quite right because the argument isn't checking anything. But I couldn't think of better wording off the top of my head. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if I change the whole arg to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would actually be better because there is an internal function already called |
||
#' contents. Defaults to `FALSE`. | ||
#' @rdname pin_read | ||
#' @export | ||
pin_write <- function(board, x, | ||
|
@@ -74,7 +78,8 @@ pin_write <- function(board, x, | |
metadata = NULL, | ||
versioned = NULL, | ||
tags = NULL, | ||
...) { | ||
..., | ||
check_hash = FALSE) { | ||
ellipsis::check_dots_used() | ||
check_board(board, "pin_write", "pin") | ||
|
||
|
@@ -111,6 +116,16 @@ pin_write <- function(board, x, | |
) | ||
meta$user <- metadata | ||
|
||
if (check_hash) { | ||
old_hash <- possibly_pin_meta(board, name)$pin_hash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we think this might fail? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to let someone use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmm, I think I'd prefer an explicit |
||
if (!is.null(old_hash) && old_hash == meta$pin_hash) { | ||
juliasilge marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cli::cli_warn( | ||
"The hash of pin {.val {name}} has not changed and will not be stored." | ||
) | ||
return(invisible(name)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could return something different here, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's correct; it should return what |
||
} | ||
} | ||
|
||
name <- pin_store(board, name, path, meta, versioned = versioned, x = x, ...) | ||
pins_inform("Writing to pin '{name}'") | ||
invisible(name) | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider defaulting this to
TRUE
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's try it. A release is still a little ways out so we can see if it is a problem in an expected way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually @hadley the other reason I had not set it to
TRUE
originally is it adds one additional call topin_meta()
. Do we think this is OK? All the API calls can be especially painful on Connect, but maybe adding yet one more set isn't much marginal difference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm, I think it's probably still worth it. It definitely saves you a lot of time/space in the case where the data hasn't changed. We'll definitely need to promote that change heavily.