Issue 397 drop httr

[couthcommander]

issue #397

Proof of concept for replacing "httr" for "curl". "RCurl" may be an even better option? Tests do not reflect new methods and behaviour (yet).

[couthcommander]

Some comments

• Shawn noted "I looked at folding httr into our package and those 6 dependencies pull in a tree of files.". This is very true, though many of these can be eliminated (.curlPost can be streamlined).
• We have additional control by replacing httr::POST with .curlPost
• The option "httr_config" will need a replacement.
• Removing the "httr" dependency adds three (these are imported by "httr"):
  • curl
  • jsonlite (parsing JSON in exportDataQuality)
  • mime (file upload)
• RCurl might have the same issue
  • at least it has RCurl::guessMIMEType
• Using "httr" makes it easier to add new functionality (all of the glue is in place).

[couthcommander]

Along with tests, some documentation would need updates (references to httr::POST)

[spgarbet]

It's looking really good now.

• Tests to be developed for 3.0.0 release (9):
  test-151-exportRecords-ArgumentValidation.R:6:5,
  test-151-exportRecords-Functionality.R:6:5,
  test-152-exportRecordsWithDags.R:6:5,
  test-153-exportRecordsWithEvents.R:6:5,
  test-155-exportRecordsWithRepeatingInstruments.R:6:5,
  test-180-exportRecordsOffline-ArgumentValidation.R:6:5,
  test-180-exportRecordsOffline-Functionality.R:6:5,
  test-190-exportReports-ArgumentValidation.R:6:5,
  test-190-exportReports-Functionality.R:6:5

[ FAIL 0 | WARN 0 | SKIP 9 | PASS 2072 ]

[spgarbet]

I'll look into documentation changes.

[spgarbet]

@obregos @jubilee2 @nutterb This is a major change to the package that gets rid of httr dependence with is deprecated. It avoids going httr2 with massive dependencies chains by going directly with curl. As such it needs to have code reviewed and agreement that this is the direction we should be moving. This PR is ready for deeper review as tests are passing and documentation is updated.

[obregos]

I get the following when running tests:
[ FAIL 0 | WARN 0 | SKIP 13 | PASS 2064 ]

I agree that we should move towards removing dependencies that are deprecated and avoiding introducing new dependencies.

[nutterb]

I remember reading about httr2 when it was first released and did a snort laugh because the dependency chain on it was so huge for what it does. But I definitely considered the httr problem to be a down-the-road problem.

Some things I've observed in reviewing this code

• the number of dependencies grew in total size, but all of the new dependencies were dependencies of httr. The net effect is the dependency chain is almost the same, but having cut out a middle man.
• You did this having only changed 23 files!? That's really impressive!
• With the exception of curl.R, none of the files have code changes that I would even notice. The new curl functions introduced are appropriately named and flow very naturally in the existing code.

If I were to go all out on nit picking this, I might say curl.R could use more white space (There are some places with multiple argument on one line, and I have a strong preference for one argument per line), but that is so cosmetic that I feel silly bringing it up.

I haven't run the test suite independently. But with two users having reported successful tests I'm not too concerned with that. I'm updating my environment right now and could run tests tomorrow. But if you're wanting an reviewer to approve, I'd be comfortable signing this now. It's quality work.

[jubilee2]

I briefly reviewed this pull request and noticed the following:

1. Legacy httr_option Variable: There is a legacy httr_option variable present in the code. It may need to be updated or replaced with the new curl configurations.

2. Further Review Needed: I can conduct a more in-depth code review tomorrow if it is still required.

[spgarbet]

I did the OCDP edit in response to Benjamin;s comment. The httr bit comment from Jubilee requires a bit more thought. Do we want to move to curl_options for the name? If we do will we provided legacy support for httr_options? How many users use these settings? There are several functions around setting the default timeout to 300s that I have an intuition could be simplified.

[spgarbet]

I have followed the config information through the code and I think there are issues. It has grown a bit convoluted with some of the refactors.

How I think it should work (and I could be wrong):

1. On connection creation, the curl config is created and stored in the connection. This connection config is a modifyList combination of default timeout, getOption('httr_config') and finally anything the user passed in to use as the config.
2. A makeApiCall pulls the config from the connection and merges with any config passed to the call and passes that down to curl.

This gets rid of any modifications being done to httr_config at the user level, always uses the connection as the default config effectively caching it. I think this would be straightforward and simpler.

[spgarbet]

Last licks. Looks good to me. Biggest change is the rcon$config is now the template packet for makeApiCall.

[jubilee2]

I get the following when running tests:
[ FAIL 0 | WARN 0 | SKIP 13 | PASS 2067 ]