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

Revamp board_url() to handle manifest files #681

Merged
merged 25 commits into from Dec 16, 2022
Merged

Conversation

juliasilge
Copy link
Member

Related to #661

Closes #650

@juliasilge
Copy link
Member Author

I was having a hard time with the tests until I remembered about is_testing(), as outlined in #640. Turns out, this setup can be a problem for what we want to do internally as well.

@juliasilge juliasilge marked this pull request as ready for review December 12, 2022 20:31
@juliasilge
Copy link
Member Author

@ijlyttle do you have time to install this and check it out? Also give me any feedback or thoughts on the code?

@ijlyttle
Copy link
Contributor

Hi @juliasilge - this is great news! I'll go through this today!

Copy link
Contributor

@ijlyttle ijlyttle left a comment

Choose a reason for hiding this comment

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

Hi @juliasilge - this looks great!

I also took it for a spin and it works just as I expect. I left a few very picky comments, most of it deals with faults in the original material.

As an aside, the organization is much improved from the original PR :)

Thanks so much for taking on these strange ideas and for being patient with my "first-draft" code. I remain at your service to "help" with the Python side!

R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
@juliasilge
Copy link
Member Author

juliasilge commented Dec 13, 2022

Thank you again so much @ijlyttle! 🙌

I really appreciate you being willing to put your thoughts down in the draft PR, and for reviewing. It did end up being a fairly significant set of changes so I'm glad we broke it up!

A question for you ❓
Do you think we should add a small vignette outlining how to use the manifest file and then having collaborators download via board_url()? Would you be interested in contributing such a vignette? You could put the webfakes bit in an echo = FALSE chunk and say in words, "and then you serve the board".

@ijlyttle
Copy link
Contributor

It would be my pleasure to contribute such a vignette! If you like, I'll wait for the dust to settle on this PR, then start another PR.

R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/testthat.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
juliasilge and others added 5 commits December 14, 2022 12:49
Co-authored-by: Hadley Wickham <h.wickham@gmail.com>
Merge branch 'board-url-versioned' of https://github.com/rstudio/pins-r into board-url-versioned

# Conflicts:
#	R/board_url.R
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
Comment on lines +21 to +25
#' - Unnamed character scalar, i.e. **a single URL** to a
#' [manifest file][write_board_manifest()]: If the URL ends in a `/`,
#' `board_url()` will look for a `_pins.yaml` manifest. If the manifest
#' file parses to a named list, versioning is supported. If it parses to a
#' named character vector, the board will not support versioning.
Copy link
Member

Choose a reason for hiding this comment

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

Why would you point it to something other than a directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is possible to pass in a URL to a file that is a manifest file but not named _pins.yaml, like this:

library(pins)
b1 <- board_folder(withr::local_tempdir(), versioned = TRUE)
b1 %>% pin_write(1:10, "x", type = "json")
#> Creating new version '20221215T205631Z-c3943'
#> Writing to pin 'x'
b1 %>% pin_write(11:20, "y", type = "json")
#> Creating new version '20221215T205631Z-cba09'
#> Writing to pin 'y'
b1 %>% pin_write(1:20, "y", type = "csv")
#> Creating new version '20221215T205631Z-5026d'
#> Writing to pin 'y'
write_board_manifest(b1)
#> Manifest file written to root folder of board, as `_pins.yaml`
fs::file_move(fs::path(b1$path, "_pins.yaml"), fs::path(b1$path, "_different.yaml"))
b1_path <- fs::path(b1$path)

b1_server <- webfakes::new_app()
b1_server$use(webfakes::mw_static(root = b1_path))
b1_process <- webfakes::new_app_process(b1_server)

b2 <- board_url(fs::path(b1_process$url(), "_different.yaml"))
b2 %>%
  pin_read("x") 
#> → <http:/127.0.0.1:64054/x/20221215T205631Z-c3943/data.txt> is not cacheable
#> → <http:/127.0.0.1:64054/x/20221215T205631Z-c3943/x.json> is not cacheable
#>  [1]  1  2  3  4  5  6  7  8  9 10

Created on 2022-12-15 with reprex v2.0.2

I'm sure it won't be the most common use case but seems like some nice flexibility to offer.

R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
R/board_url.R Outdated Show resolved Hide resolved
juliasilge and others added 5 commits December 15, 2022 13:50
@juliasilge juliasilge merged commit fd1708f into main Dec 16, 2022
@juliasilge juliasilge deleted the board-url-versioned branch December 16, 2022 01:06
This was referenced Dec 17, 2022
@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 Dec 31, 2022
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.

None yet

3 participants