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

Added retry_non_idempotent=true to HTTP post requests #9

Merged
merged 2 commits into from
May 22, 2018

Conversation

mcmcgrath13
Copy link
Contributor

@mcmcgrath13 mcmcgrath13 commented May 18, 2018

Added retry_non_idempotent=true to HTTP post requests

Types of changes

This PR implements the following changes:
(Please tick any or all of the following that are applicable)

  • ✨ New feature (A non-breaking change which adds functionality).
  • 🐛 Bug fix (A non-breaking change, which fixes an issue).
  • 💥 Breaking change (fix or feature that would cause existing functionality to change).

📋 Additional detail

  • If you have implemented new features or behaviour
    • Provide a description of the addition in as many details as possible.
      On efetch and esearch added named variable of retry_non_idempotent set to true (default is false). As these are post requests HTTP is not retrying them if eutils is non-responsive, but as they are functionally get requests there is no harm in resending them

This was change was recommended as a fix to this issue here

  • Provide justification of the addition.
    This was causing issues in BioMedQuery where many eutils requests are made. Specifically when efetching with a list of PMIDs.

  • Provide a runnable example of use of your addition. This lets reviewers
    and others try out the feature before it is merged or makes it's way to release.
    There is no noticeable change to functionality, behavior, just less likely to get an IOError from HTTP.

  • If you have changed current behaviour...

    • Describe the behaviour prior to you changes
      There is no noticeable change to functionality, behavior, just less likely to get an IOError from HTTP.

    • Describe the behaviour after your changes and justify why you have made the changes,
      Please describe any breakages you anticipate as a result of these changes.
      There is no noticeable change to functionality, behavior, just less likely to get an IOError from HTTP.

    • Does your change alter APIs or existing exposed methods/types?
      If so, this may cause dependency issues and breakages, so the maintainer
      will need to consider this when versioning the next release.
      No.

    • If you are implementing changes that are intended to increase performance, you
      should provide the results of a simple performance benchmark exercise
      demonstrating the improvement. Especially if the changes make code less legible.

☑️ Checklist

  • 🎨 The changes implemented is consistent with the julia style guide.
  • 📘 I have updated and added relevant docstrings, in a manner consistent with the documentation styleguide.
  • 📘 I have added or updated relevant user and developer manuals/documentation in docs/src/.
  • 🆗 There are unit tests that cover the code changes I have made.
  • 🆗 The unit tests cover my code changes AND they pass.
  • 📝 I have added an entry to the [UNRELEASED] section of the manually curated CHANGELOG.md file for this repository.
  • 🆗 All changes should be compatible with the latest stable version of Julia.
  • 💭 I have commented liberally for any complex pieces of internal code.

@codecov-io
Copy link

codecov-io commented May 18, 2018

Codecov Report

Merging #9 into master will decrease coverage by 49.6%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master       #9       +/-   ##
===========================================
- Coverage   90.55%   40.94%   -49.61%     
===========================================
  Files           2        2               
  Lines         127      127               
===========================================
- Hits          115       52       -63     
- Misses         12       75       +63
Impacted Files Coverage Δ
src/umls.jl 0% <0%> (-86.31%) ⬇️
src/eutils.jl 96.29% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5066442...20d7b49. Read the comment docs.

@mcmcgrath13
Copy link
Contributor Author

@benjward Is anything else needed to merge the request? I don't see a CHANGELOG.md file.

@mirestrepo mirestrepo merged commit 2677c7a into BioJulia:master May 22, 2018
@TransGirlCodes
Copy link
Member

@mcmcgrath13 That's ok, a few of the repos have changelogs but not all of them yet. So long as @mirestrepo is happy that everything else checks out I wouldn't worry about it too much a changelog can always be retroactively added.

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.

4 participants