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

New force_identical_write arg #735

Merged
merged 12 commits into from
May 5, 2023
8 changes: 8 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
# pins (development version)

## Breaking changes

* `pin_write()` no longer writes identical pin contents by default, and gains a
`force_identical_write` argument for writing even when the pin contents are
identical to the last version (#735).

## Other improvements

* The `print` method for boards no longer calls `pin_list()` internally (#718).

* `board_s3()` now uses pagination for listing and versioning (#719, @mzorko).
Expand Down
2 changes: 2 additions & 0 deletions R/pin-meta.R
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
17 changes: 16 additions & 1 deletion R/pin-read-write.R
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ 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 force_identical_write Store the pin even if the pin contents are
#' identical to the last version (compared using the hash). Only the pin
#' contents are compared, not the pin metadata. Defaults to `FALSE`.
#' @rdname pin_read
#' @export
pin_write <- function(board, x,
Expand All @@ -74,7 +77,8 @@ pin_write <- function(board, x,
metadata = NULL,
versioned = NULL,
tags = NULL,
...) {
...,
force_identical_write = FALSE) {
ellipsis::check_dots_used()
check_board(board, "pin_write", "pin")

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

if (!force_identical_write) {
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

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 (identical(old_hash, meta$pin_hash)) {
pins_inform(c(
"!" = "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
10 changes: 10 additions & 0 deletions R/testthat.R
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,16 @@ test_api_versioning <- function(board) {
)
})

testthat::test_that("force_identical_write arg skips an identical subsequent write", {
orig <- local_pin(board, 1)
name <- local_pin(board, 1, force_identical_write = TRUE)
ui_loud()
testthat::expect_message(
pin_write(board, 1, name),
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
4 changes: 2 additions & 2 deletions R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ modifyList <- function(x, y) {

last <- function(x) x[[length(x)]]

pins_inform <- function(...) {
pins_inform <- function(msg) {
opt <- getOption("pins.quiet", NA)
if (identical(opt, FALSE) || (identical(opt, NA))) {
inform(glue(..., .envir = caller_env()))
cli::cli_inform(msg, .envir = caller_env())
}
}

Expand Down
7 changes: 6 additions & 1 deletion man/pin_read.Rd

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

4 changes: 2 additions & 2 deletions tests/testthat/_snaps/board_folder.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@
Creating new version '20130104T050607Z-xxxxx'
Writing to pin 'x'
Code
pin_write(b, 1:5, "x", type = "rds")
pin_write(b, 1:6, "x", type = "rds")
Message
Replacing version '20130104T050607Z-xxxxx' with '20130204T050607Z-yyyyy'
Writing to pin 'x'
Code
pin_write(b, 1:6, "x", type = "rds")
pin_write(b, 1:7, "x", type = "rds")
Message
Replacing version '20130204T050607Z-yyyyy' with '20130304T050607Z-zzzzz'
Writing to pin 'x'
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/_snaps/pin_versions.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

Code
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x", force_identical_write = TRUE)
Condition
Error in `pin_store()`:
! The new version "20120304T050607Z-xxxxx" is the same as the most recent version.
Expand Down
4 changes: 2 additions & 2 deletions tests/testthat/test-board_connect.R
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@ test_that("can update access_type", {
# default is acl
expect_equal(rsc_content_info(board, guid)$access_type, "acl")

pin_write(board, 1:5, name, access_type = "logged_in")
pin_write(board, 1:6, name, access_type = "logged_in")
expect_equal(rsc_content_info(board, guid)$access_type, "logged_in")

# writing again doesn't change the access_type
pin_write(board, 1:5, name)
pin_write(board, 1:7, name)
expect_equal(rsc_content_info(board, guid)$access_type, "logged_in")
})

Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/test-board_folder.R
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ test_that("generates useful messages", {
ui_loud()
b <- board_temp()
expect_snapshot({
pin_write(b, 1:5, "x", type = "rds")
pin_write(b, 1:5, "x", type = "rds")
pin_write(b, 1:6, "x", type = "rds")
pin_write(b, 1:7, "x", type = "rds")
Copy link
Member Author

Choose a reason for hiding this comment

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

I did have to change a couple of tests if the default is compare_hash = TRUE

})
})
2 changes: 1 addition & 1 deletion tests/testthat/test-pin_versions.R
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ test_that("informative error for writing with same version", {
board <- board_temp(versioned = TRUE)
expect_snapshot(error = TRUE, {
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x")
board %>% pin_write(1:10, "x", force_identical_write = TRUE)
})
})

Expand Down