rstudio / pins-r

Pin, discover, and share resources
https://pins.rstudio.com
Other
312 stars 63 forks source link

`pin_download_one()` defines `skip_download` without using it #472

Closed atusy closed 3 years ago

atusy commented 3 years ago

The function checks if Cache-Control: max-age is valid, and sets skip_download <- FALSE

https://github.com/rstudio/pins/blob/322b3df464202ef3a4726cded3f1a549cefef11c/R/legacy_pin_download.R#L104

However, this value is not used anywhere in the function.

I guess || !skip_download should be added to the if statement below.

https://github.com/rstudio/pins/blob/322b3df464202ef3a4726cded3f1a549cefef11c/R/legacy_pin_download.R#L125-L126

hadley commented 3 years ago

This function is basically superseded so I don’t have plans to fix any bugs in it.

atusy commented 3 years ago

Okay, thank you. I reported just because I noticed through reading the code just to understand cache validation mechanism (not to use).

hadley commented 3 years ago

Yeah, I'd recommend reading through http_download() or possibly https://github.com/r-lib/httr2/blob/master/R/req-cache.R (the logic is harder to follow there because it's interwoven with the httr2 download loop, but it's a bit more recent and I think a bit better tested).

atusy commented 3 years ago

Thanks, I will!

github-actions[bot] commented 2 years ago

This issue 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.