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

fix: Handle exceptions raised by request.urlopen in HttpClient.post #17

Merged
merged 1 commit into from
Apr 21, 2022
Merged

fix: Handle exceptions raised by request.urlopen in HttpClient.post #17

merged 1 commit into from
Apr 21, 2022

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Apr 20, 2022

Summary

At the moment the sad path is essentially non-functional, as request.urlopen raises HTTPError exceptions for responses with status code 400...500. This PR fixes buggy exception handling in HttpClient.post function making it work as expected.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: No

src/amplitude/http_client.py Show resolved Hide resolved
src/amplitude/http_client.py Outdated Show resolved Hide resolved
src/amplitude/http_client.py Outdated Show resolved Hide resolved
@bohan-amplitude
Copy link
Contributor

@Sija Thank you for helping us and fix the bug here. I'm working on adding more test case and find/fix potential bugs.

@bohan-amplitude bohan-amplitude merged commit 4150ea0 into amplitude:main Apr 21, 2022
@Sija
Copy link
Contributor Author

Sija commented Apr 21, 2022

@bohan-amplitude Could this be released as a bugfix release anytime soon? I'm asking since this prevents us from using this library at the moment.

@bohan-amplitude
Copy link
Contributor

@Sija Just released this to PyPi. Version 0.2.1 should be available now.

@Sija
Copy link
Contributor Author

Sija commented Apr 21, 2022

@bohan-amplitude Great, thanks! 🙏

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.

None yet

2 participants