Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update requests to use httr::RETRY and harmonise verbose messages #271

Merged
merged 47 commits into from
Sep 17, 2020

Conversation

stitam
Copy link
Contributor

@stitam stitam commented Jul 13, 2020

This PR addresses issues #257 and #268.

This PR updates most webservice requests to use httr::RETRY(). The main benefit is that it allows a webservice to be queried multiple times when the previous attempts were unsuccessful. This is especially useful when some webservices randomly become unavailable for a few seconds. Another benefit of the function is that it assigns HTTP status codes and status messages to all webservice requests, allowing harmonised verbose messages.

Feel free to add a comment/review/suggest alternatives.

PR task list:

  • Update NEWS
  • Add tests (if appropriate)
  • Update documentation with devtools::document()
  • Check package passed

@stitam stitam marked this pull request as ready for review August 31, 2020 21:39
@stitam
Copy link
Contributor Author

stitam commented Aug 31, 2020

I didn't touch ChemSpider, because I figured the functions can be simplified quite a bit. It's already done but I'll open a separate PR for that.

@stitam stitam marked this pull request as draft September 1, 2020 12:46
@stitam
Copy link
Contributor Author

stitam commented Sep 2, 2020

Travis is not working because Trusty is no longer supported, and I cannot get rJava to work under more recent versions. I'd rather let it go and fix Travis in a separate PR.

@stitam stitam marked this pull request as ready for review September 2, 2020 08:26
@stitam stitam added this to the RC2019F milestone Sep 5, 2020
Copy link
Contributor

@andschar andschar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks nice! Some minor comments.

CONTRIBUTING.md Outdated Show resolved Hide resolved
R/chebi.R Outdated Show resolved Hide resolved
R/chemid.R Outdated Show resolved Hide resolved
R/chemid.R Show resolved Hide resolved
R/cts.R Outdated Show resolved Hide resolved
R/ping.R Outdated Show resolved Hide resolved
R/pubchem.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love love love webchem_url(), and webchem_message(), and I love that now all of the functions use the same httr style including the HTML status reporting. Wonderful stuff. However, for multiple queries I still think most users would prefer that a function fail quickly if a service is down, rather than going very slowly and returning many NAs.

If you decide to keep this behavior, however, there are a number of places where the function still errors when a service is down (tested by turning off wifi and running example). I think it almost always happens because some tidying step or row-binding step is expecting something other than a length 1, unnamed vector. This could be fixed by returning a different data structure inside the if (inherits(res, "try-error")) statement. Alternatively, you could wrap foo() in try() or use purrr::possibly() rather than wrapping RETRY() in try() inside of foo(). So for example:

db_query <- function(query, verbose = TRUE) {

  foo <- function(query) {
    <function to do a single query.  Errors when service is down>
  }
  foo_safe <- possibly(foo, otherwise = tibble(query = NA, var1 = NA, var2 = NA), quiet = !verbose)
  out <- lapply(query, foo_safe)
}

R/cts.R Show resolved Hide resolved
R/alanwood.R Show resolved Hide resolved
R/chebi.R Outdated Show resolved Hide resolved
R/cir.R Show resolved Hide resolved
R/etox.R Outdated Show resolved Hide resolved
R/ping.R Outdated Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
R/wikidata.R Outdated Show resolved Hide resolved
R/wikidata.R Show resolved Hide resolved
@stitam stitam merged commit 0cbdb29 into ropensci:master Sep 17, 2020
@stitam stitam deleted the retry branch September 17, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants