Implement functionality with 'curl' package from CRAN #29

Closed
restonslacker opened this Issue Jan 20, 2017 · 21 comments

Projects

None yet

3 participants

@restonslacker
Contributor

Since March 2014, curl was released to CRAN. I did a very small amount of testing and it seems like the system() calls to an installed version of curl could be replaced. I'm willing to work on a PR but wanted to:

  1. check to see if you were interested in going down this path
  2. understanding what issues you were running into with RCurl so i could make sure those aren't present in a refactor

sample test code:

h <- new_handle()
handle_setheaders(h, .list=list('Access-Token' = "not my actual token"))
r <- curl_fetch_memory("https://api.pushbullet.com/v2/users/me", h) 
rawToChar(r$content)

# [1] "{\"active\":true,\"iden\":\"blah\",\"created\":1.4068562807078998e+09,\"modified\":1.4818938567380302e+09,\"email\":\"wenchel@gmail.com\",\"email_normalized\":\"wenchel@gmail.com\",\"name\":\"Seth Wenchel\",\"image_url\":\"https://static.pushbullet.com/google-user/foobar\",\"max_upload_size\":26214400}"
@eddelbuettel
Owner

I would take a PR. I just couldn't be bothered -- using /usr/bin/curl is trivial for me, but there is of course the issue of an external process each time which is a slight smell.

Would be able to also extend the test set to really make sure we are not creating a regression?

@restonslacker
Contributor

I understand. My new work computer is windows and i don't have admin rights so getting a curl.exe binary installed was a bit of work. My hope is that switching to the curl package would make it easier for others.

What do you mean by "extend the test set?" My cursory inspection of simpleTests.R makes me think that changing the functions would all still work after refactoring with curl. Were you referring to my code block above? That was just a quick demonstration to myself that this idea might be feasible.

@eddelbuettel
Owner

I just want to be 300% sure we are not breaking something. I don't have that many test cases myself. And maybe simpleTests.R is reasonably complete. Maybe I should start by turning code coverage on.

BTW appreciate a) your interest and b) the fact that you file issues before blindsiding me with a PR (as happened today in another repo).

@restonslacker
Contributor
restonslacker commented Jan 20, 2017 edited

I agree. I appreciate the general level of stability in the R community and would like to avoid breaking anything here.

In fairness, I did blindside you with a different PR this morning in this repo. I realized I should have opened an issue for it after I sent it, but it was standalone and a small amount of work compared to what's proposed here.

tasks for curl:

  • replace setup in .onAttach()
  • remove all calls to .getCurl() (should address #26, as well)
  • convert all calls of form txt <- sprintf(...); system(txt) to use curl_fetch_memory()

tasks for simpleTests.R:

  • ensure all current tests still pass
  • review for missing test cases
  • investigate if something like secure would be good for storing a test.json config file
  • investigate code coverage options like covr. (NB: this is new to me. if you use something else, please let me know)
@eddelbuettel
Owner

Very good. Setting up the covr package is independent of this and I'll take care of it; ditto with the dead links in README.md. (CRAN checks would have found them on the next upload; they follow all links which is good practice.)

@restonslacker
Contributor

The type address does not appear in the current set of api docs and is listed as having been removed as of 28 Jan 2015. I'm going to remove support from the package as testing has show them to no longer be supported.

Of course Pushbullet still has an address push example in their iOS section...

@restonslacker
Contributor

do you have a preference for whether this should be an error or a warning?

@eddelbuettel
Owner

warning() seems better. Users can then decide if they want "warnings-as-error" which R supports.

@eddelbuettel
Owner

Cannot really do coverage testing ... for the same reason we do not test Travis: need a token and account / channel to talk to 😢

@eddelbuettel
Owner

Or rather, can do coverage testing the way I have done for another package ... by manually uploading rather than having Travis do it.

@restonslacker
Contributor
@eddelbuettel
Owner

Sadly way more pedestrian:

  1. Set the required environment variable: Sys.setenv("Run_RPushbullet_Tests"="yes")
  2. Run coverage in RStudio (because it is so easy to then look at it via shiny): cvr <- package_coverage()
  3. Push to website using helper function: codecov(coverage=cvr, token=tokstr)

where tokstr is something you get at codecov.io when you add a new repo.

@eddelbuettel
Owner

It is still building at their end but some things are here: https://codecov.io/gh/eddelbuettel/rpushbullet

@restonslacker restonslacker added a commit to restonslacker/rpushbullet that referenced this issue Jan 23, 2017
@restonslacker restonslacker initial refactor to use CRAN package CURL. (#29) links & notes workin…
…g. still an issue with file uploads.
c5f034a
@restonslacker
Contributor

links & notes are working, but i'm having an issue with files. i keep getting a status code of 404 when it tries to upload the file before the push. Do you have time a take a look and see if it's something obvious that I missed? I'll try to get some more time this afternoon to look at it more closely if you don't.

ICYNI: relevant part from curl package manual about posting forms: https://cran.r-project.org/web/packages/curl/vignettes/intro.html#posting_forms

@eddelbuettel
Owner

I know next to nothing about internals of these things and protocols -- which is why I stuck with using the curl binary. It works.

Now, if you cook up a real small example, ideally even without reliance on RPushBullet and its token, @jeroenooms maybe be able to help you from the curl (the package, not the binary) side.

@restonslacker restonslacker added a commit to restonslacker/rpushbullet that referenced this issue Jan 23, 2017
@restonslacker restonslacker solved issue with file uploads. PEBKAC #29 47ec713
@restonslacker
Contributor

ok. all of the tests in simpleTests.R now pass. I'm going to go ahead and open a PR.

@eddelbuettel
Owner

Hooray for self-diagnosed PEBKACs :)

@restonslacker
Contributor

WRT: "investigate if something like secure would be good for storing a test.json config file", secure does not seem like a good option as it is not maintained and seems to have some issues if you want to use keys that are not stored in ~/.shh/id_rsa.

@gaborcsardi mentioned that he was re-implementing the functionality, but i don't know if he intends to release it as a package. currently there does not appear to be a github repo for it. see: hadley/secure#15

@gaborcsardi

No, I don't have anything available, sorry. But you can just use the openssl package directly.

@restonslacker
Contributor

Ok. Thanks @gaborcsardi for responding.

@eddelbuettel
Owner

Done in #30.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment