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

Change the pin_write() function signature, moving the dots #792

Merged
merged 8 commits into from
Sep 29, 2023

Conversation

juliasilge
Copy link
Member

@juliasilge juliasilge commented Sep 29, 2023

We've been talking about ... being in the wrong place for quite a while, and we think it's time to rip the band-aid off for a better experience for people.

Technically we "should" put the dots before name but it is very very very common for folks to provide name by position (without name = ). We do it in all our docs and tests, and I believe it is very common with users as well. I would like to propose that we move the dots to between name and type, as it will not be too disruptive for users and we'll be able to get to our better situation of moving around or adding args after the dots.

The new error that folks see if they are passing by position is this:

library(pins)
board <- board_temp()
board |> pin_write(10:20, "nice-numbers", "json", "my great title")
#> Error in `pin_write()`:
#> ! Arguments after the dots `...` must be named, like `type = "json"`.
#> Backtrace:
#>     ▆
#>  1. └─pins::pin_write(board, 10:20, "nice-numbers", "json", "my great title")
#>  2.   └─cli::cli_abort("Arguments after the dots `...` must be named, like {.code type = \"{dots[[1]]}\"}.") at pins-r/R/pin-read-write.R:85:4
#>  3.     └─rlang::abort(...)

Created on 2023-09-29 with reprex v2.0.2

The message doesn't make quite as much sense if people are using a mix of calling by name and position, but surely this must be rare:

library(pins)
board <- board_temp()
board |> pin_write(10:20, name = "nice-numbers", type = "json", "my great title")
#> Error in `pin_write()`:
#> ! Arguments after the dots `...` must be named, like `type = "my great
#>   title"`.
#> Backtrace:
#>     ▆
#>  1. └─pins::pin_write(board, 10:20, name = "nice-numbers", type = "json", "my great title")
#>  2.   └─cli::cli_abort("Arguments after the dots `...` must be named, like {.code type = \"{dots[[1]]}\"}.") at pins-r/R/pin-read-write.R:85:4
#>  3.     └─rlang::abort(...)

Created on 2023-09-29 with reprex v2.0.2

At least they are still getting an error with some helpful info, even if it isn't as fully helpful.

@juliasilge juliasilge marked this pull request as ready for review September 29, 2023 18:06
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Your reasoning makes sense to me.

force_identical_write = FALSE) {
check_board(board, "pin_write", "pin")
dots <- list2(...)
if (!is_empty(dots) && is.null(names(dots[1]))) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the logic is quite right here as names will be NULL if all the elements are unnamed, but if there's one named element, it'll be "".

I don't know if ...names() is available in old enough R for us to use it, but you could also try something like this:

if (!missing(...) && !(is.null(...names())) || ...names()[[1]] == ""))

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we do not yet have ...names() in oldrel-3 and oldrel-4.

@juliasilge juliasilge merged commit b3f1fcd into main Sep 29, 2023
14 checks passed
@juliasilge juliasilge deleted the pin-write-function-signature branch September 29, 2023 18:59
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants