search

ropensci / rnoaa

R interface to many NOAA data APIs
https://docs.ropensci.org/rnoaa
Other
330 stars 84 forks source link

Edchange #72

Closed edweber closed 9 years ago

edweber commented 9 years ago

Upon further review, my pull requests #68 and #70 were not so good. Setting blank.lines.skip=FALSE in pull #68 introduced a new bug. When results are saved to memory, the newline at the end of text parsed from a "response" object results in a record of NAs as the last row of the dataframe. Files are still saved correctly without the trailing newline by httrs:::GET, so the bug doesn't appear when files are saved to disk. Sorry, I didn't notice that earlier.

I was also thinking about Scott's comment that the httr package usually encodes URLs as needed, but was not parsing query arguments that are quoted. My pull request #70 solves the specific problem of spaces in query arguments but other characters that should be escaped would still cause the function to puke. The httr package calls RCurl::curlEscape to do the encoding, so I think calling curlEscape on the args before adding them to the url is a more correct and robust fix. This would mean making the dependency on RCurl explicit though.

sckott commented 9 years ago

@edweber Thanks for this, looks good. I was looking for a similar function in httr for RCurl::curlEscape so we don't have to import another pkg, but no dice

edweber commented 9 years ago

Hi Scott, I guess one benefit of calling rnoaa from ots would be that the noaa calcofi data are also available. Not sure how much work it would take to put that together though.

Importing RCurl is really just a formality for rnoaa because it’s required by httrs anyway. In fact, if you trace it out, rnoaa was already calling httrr, which was calling RCurl::curlEscape to do the parsing.

Ed

On Nov 10, 2014, at 12:49 PM, Scott Chamberlain notifications@github.com wrote:

@edweber Thanks for this, looks good. I was looking for a similar function in httr for RCurl::curlEscape so we don't have to import another pkg, but no dice

— Reply to this email directly or view it on GitHub.

sckott commented 9 years ago

It wouldn't be hard to import rnoaa in ots and just use it where needed in ots

Right, RCurl is essentially imported anyway, and curlEscape is already used in httr