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.

Sign up for GitHub

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

Merged
merged 12 commits into from
May 5, 2023
Merged

New force_identical_write arg #735

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b30eadd
New `check_hash` arg
juliasilge Apr 26, 2023
5180df4
Add test
juliasilge Apr 26, 2023
d313fbe
Update docs
juliasilge Apr 26, 2023
312171c
Clarify docs about no checking of metadata
juliasilge May 1, 2023
04565eb
Better warning message
juliasilge May 1, 2023
28f0ecd
Update NEWS
juliasilge May 1, 2023
856b26c
Update test
juliasilge May 1, 2023
4f219ad
Apply suggestions from code review
juliasilge May 4, 2023
7966273
Use `cli_inform()` instead of warning
juliasilge May 4, 2023
01508aa
Change arg to `compare_hash`, default of `TRUE`
juliasilge May 4, 2023
d93dab9
Update a couple of tests for default of `TRUE`
juliasilge May 4, 2023
7503901
Switch to `force_identical_write`, mark as breaking change in NEWS
juliasilge May 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

* Fixed bug in `cache_prune()` to correctly find caches for `board_url()` (#734).

* Added new `check_hash` argument when writing a pin to check whether the pin
juliasilge marked this conversation as resolved.
Show resolved Hide resolved
contents are identical to the last version (#735).

# pins 1.1.0

## Breaking changes
Expand Down
2 changes: 2 additions & 0 deletions R/pin-meta.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ pin_meta <- function(board, name, version = NULL, ...) {
UseMethod("pin_meta")
}

possibly_pin_meta <- possibly(pin_meta)

multi_meta <- function(board, names) {
meta <- map(names, possibly(pin_meta, empty_local_meta), board = board)

Expand Down
18 changes: 17 additions & 1 deletion R/pin-read-write.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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 to pin_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.

Copy link
Member

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.

#' 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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

What if I change the whole arg to compare_hash?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 check_hash(). Take a look and see if you have feedback on the new name/docs.

#' contents. Defaults to `FALSE`.
#' @rdname pin_read
#' @export
pin_write <- function(board, x,
Expand All @@ -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")

Expand Down Expand Up @@ -111,6 +116,17 @@ pin_write <- function(board, x,
)
meta$user <- metadata

if (check_hash) {
old_hash <- possibly_pin_meta(board, name)$pin_hash
Copy link
Member

Choose a reason for hiding this comment

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

Why do we think this might fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to let someone use check_hash = TRUE when they write a pin for the first time, before there is any metadata. old_hash returns as NULL here when there isn't any metadata.

Copy link
Member

@hadley hadley May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hmmm, I think I'd prefer an explicit pin_exists() check? Is there some reason not to do that?

if (!is.null(old_hash) && old_hash == meta$pin_hash) {
juliasilge marked this conversation as resolved.
Show resolved Hide resolved
cli::cli_warn(c(
juliasilge marked this conversation as resolved.
Show resolved Hide resolved
"!" = "The hash of pin {.val {name}} has not changed.",
"*" = "Your pin will not be stored."
))
return(invisible(name))
Copy link
Member Author

Choose a reason for hiding this comment

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

We could return something different here, like NULL maybe.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's correct; it should return what pin_write() usually returns.

}
}

name <- pin_store(board, name, path, meta, versioned = versioned, x = x, ...)
pins_inform("Writing to pin '{name}'")
invisible(name)
Expand Down
9 changes: 9 additions & 0 deletions R/testthat.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,15 @@ test_api_versioning <- function(board) {
)
})

testthat::test_that("check_hash arg skips an identical subsequent write", {
orig <- local_pin(board, 1, check_hash = TRUE)
name <- local_pin(board, 1)
testthat::expect_warning(
pin_write(board, 1, name, check_hash = TRUE),
regexp = "Your pin will not be stored"
)
})

testthat::test_that("unversioned write overwrites single previous version", {
name <- local_pin(board, 1)
pin_write(board, 2, name, versioned = FALSE)
Expand Down
8 changes: 7 additions & 1 deletion man/pin_read.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.