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

Raise bad response status code #12

Merged
merged 1 commit into from
Mar 15, 2018
Merged

Conversation

bmwilly
Copy link
Contributor

@bmwilly bmwilly commented Mar 14, 2018

If a request results in a non-200 status code, raise the exception
instead of returning a requests.Response.

Fixes #11

@@ -72,11 +72,11 @@ class APIClient(object):
"""

resources = {
'scopes' : {
'scopes': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are just whitespace differences to comply with PEP8.

Copy link
Contributor

@tzengerink tzengerink left a comment

Choose a reason for hiding this comment

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

Thanks for your interest in this repository and for your invested time.

Next to one minor comment on your changes, I would like to see the tests pass again. Can you have a look into that?

See: https://travis-ci.org/usabilla/api-python/jobs/353342188

usabilla.py Outdated
@@ -300,20 +297,16 @@ def item_iterator(self, url):

:type url: str

:returns: An `generator` that yeilds the requested data.
:returns: A `generator` that yeilds the requested data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, there's a typo in yields too.

Also this comment/docstring needs an addition, since the method now can also raise an HTTPError.

@bmwilly
Copy link
Contributor Author

bmwilly commented Mar 14, 2018

@tzengerink np, I should've checked that. They should pass now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 78.704% when pulling 6d6b2aa on contiamo:master into ee06476 on usabilla:master.

@coveralls
Copy link

coveralls commented Mar 14, 2018

Coverage Status

Coverage increased (+0.1%) to 78.704% when pulling 87b1011 on contiamo:master into ee06476 on usabilla:master.

Copy link
Contributor

@tzengerink tzengerink left a comment

Choose a reason for hiding this comment

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

Great, almost there!

tests.py Outdated
import usabilla as ub

from mock import Mock
import requests
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see the import statements being ordered alphabetically. Next to that it would be great to group the import ... statements and then do the from ... import ... statements.

import logging
import requests
import usabilla as ub

from mock import Mock
from unittest import TestCase, main as unittest_main

Copy link
Contributor Author

@bmwilly bmwilly Mar 15, 2018

Choose a reason for hiding this comment

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

done 👍

If a request results in a non-200 status code, raise the exception
instead of returning a `requests.Response`.

Fixes usabilla#11
Copy link
Contributor

@tzengerink tzengerink left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution, @bmwilly!

@tzengerink tzengerink merged commit 04d2973 into usabilla:master Mar 15, 2018
@bmwilly
Copy link
Contributor Author

bmwilly commented Mar 15, 2018

@tzengerink thanks for the speedy review + merge!

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

3 participants