gesistsa / rio

🐟 A Swiss-Army Knife for Data I/O

http://gesistsa.github.io/rio/

600 stars 76 forks source link

`remote_to_local` requires refactoring #403

Closed chainsawriot closed 5 months ago

chainsawriot commented 5 months ago

The two large if blocks and the repeated handling of google urls scream refactoring.

chainsawriot commented 5 months ago

This bunch of code is not tested

https://github.com/gesistsa/rio/blob/144ebca1e337654ac7a9c7492082a9a25b558b12/R/remote_to_local.R#L34-L52

chainsawriot commented 5 months ago

The logic in this bunch of code looks incorrect to me. But I don't have a URL that can test this code.

https://github.com/gesistsa/rio/blob/019f0e4c1f8f60dcdded1ddc394d4434d1b4ec58/R/remote_to_local.R#L43-L52

chainsawriot commented 5 months ago

36 archeological dig. And the commit closing this 2172307 has no tests.

I am asking myself: is it popular to have a URL of a file without a file extension, even with the final redirected url, but with the Content Disposition? If it is really the case, is it still rio's responsibility to stiff the curl HEADER for it's file format? It appears to go back to #238.

chainsawriot commented 5 months ago

A colleague pointed me to this MDN article.

The Content-Disposition header is defined in the larger context of MIME messages for email, but only a subset of the possible parameters apply to HTTP forms and POST requests. Only the value form-data, as well as the optional directive name and filename, can be used in the HTTP context.

Probably, it is only for a URL that is like a HTTP form request.