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(response): reoder exception handling to correctly parse a broken response #164

Merged
merged 10 commits into from
Sep 9, 2024

Conversation

SlowMo24
Copy link
Collaborator

No description provided.

@SlowMo24 SlowMo24 self-assigned this Jul 31, 2024
Copy link
Contributor

@matthiasschaub matthiasschaub left a comment

Choose a reason for hiding this comment

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

I left two minor suggestions.

In general, it would be nice to do only one thing in the try-block.

Then it is easier to parse the exceptions. What can go wrong where.

ohsome/test/test_exceptions.py Outdated Show resolved Hide resolved
ohsome/test/test_exceptions.py Outdated Show resolved Hide resolved
@SlowMo24
Copy link
Collaborator Author

SlowMo24 commented Aug 7, 2024

In general, it would be nice to do only one thing in the try-block.

Then it is easier to parse the exceptions. What can go wrong where.

You suggest placing line 341 https://github.com/GIScience/ohsome-py/pull/164/files#diff-0684a040fd4741e6267194be04e688f118c72fdae4256902f407e9a6c11dac2aL341 into a separate try block?

@SlowMo24 SlowMo24 requested a review from matthiasschaub August 7, 2024 16:09
@matthiasschaub
Copy link
Contributor

In general, it would be nice to do only one thing in the try-block.
Then it is easier to parse the exceptions. What can go wrong where.

You suggest placing line 341 https://github.com/GIScience/ohsome-py/pull/164/files#diff-0684a040fd4741e6267194be04e688f118c72fdae4256902f407e9a6c11dac2aL341 into a separate try block?

Yes, or even:

response = self._session().post(url=self._url, data=self._parameters)
try:
    response.raise_for_status(
except:
    ...
try:
    response.json()
except:
    ...

So that one knows that exceptions caught in one block belong only to the http response or json parsing and not to _session().post.

SlowMo24 and others added 4 commits September 3, 2024 11:11
Co-authored-by: Matthias (~talfus-laddus) <[email protected]>
Co-authored-by: Matthias (~talfus-laddus) <[email protected]>
…166)

Bumps the pip group with 1 update: [notebook](https://github.com/jupyter/notebook).


Updates `notebook` from 7.2.1 to 7.2.2
- [Release notes](https://github.com/jupyter/notebook/releases)
- [Changelog](https://github.com/jupyter/notebook/blob/@jupyter-notebook/[email protected]/CHANGELOG.md)
- [Commits](https://github.com/jupyter/notebook/compare/@jupyter-notebook/[email protected]...@jupyter-notebook/[email protected])

---
updated-dependencies:
- dependency-name: notebook
  dependency-type: indirect
  dependency-group: pip
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@SlowMo24 SlowMo24 force-pushed the test_brokne_response branch from 8da315d to 486de18 Compare September 3, 2024 09:11
@SlowMo24
Copy link
Collaborator Author

SlowMo24 commented Sep 3, 2024

In general, it would be nice to do only one thing in the try-block.
Then it is easier to parse the exceptions. What can go wrong where.

You suggest placing line 341 https://github.com/GIScience/ohsome-py/pull/164/files#diff-0684a040fd4741e6267194be04e688f118c72fdae4256902f407e9a6c11dac2aL341 into a separate try block?

Yes, or even:

response = self._session().post(url=self._url, data=self._parameters)
try:
    response.raise_for_status(
except:
    ...
try:
    response.json()
except:
    ...

So that one knows that exceptions caught in one block belong only to the http response or json parsing and not to _session().post.

thanks, I had the same gut feeling during coding but thought it was too complicated. I have made this change now, please re-review

Copy link
Member

@redfrexx redfrexx left a comment

Choose a reason for hiding this comment

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

Looks good, as far as I can see. Thanks!

@SlowMo24 SlowMo24 merged commit b27c519 into master Sep 9, 2024
4 checks passed
@SlowMo24 SlowMo24 deleted the test_brokne_response branch September 9, 2024 12:32
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