-
Notifications
You must be signed in to change notification settings - Fork 29
fix: more consistent naming of HTTP status code related attributes #185
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import warnings | ||
from http import HTTPStatus | ||
from typing import Optional | ||
|
||
|
@@ -39,15 +40,28 @@ def __init__(self, code: int, *, message: Optional[str] = None, http_response: O | |
# Call the base class constructor with the parameters it needs | ||
super().__init__(message) | ||
self.message = message | ||
self.code = code | ||
self.status_code = code | ||
self.http_response = http_response | ||
self.global_transaction_id = None | ||
if http_response is not None: | ||
self.global_transaction_id = http_response.headers.get('X-Global-Transaction-ID') | ||
self.message = self.message if self.message else self._get_error_message(http_response) | ||
|
||
# pylint: disable=fixme | ||
# TODO: delete this by the end of 2024. | ||
@property | ||
def code(self): | ||
"""The old `code` property with a deprecation warning.""" | ||
|
||
warnings.warn( | ||
'Using the `code` attribute on the `ApiException` is deprecated and' | ||
'will be removed in the future. Use `status_code` instead.', | ||
DeprecationWarning, | ||
) | ||
return self.status_code | ||
|
||
Comment on lines
+50
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought it would be good to not remove the old property completely, but mark it deprecated and give some time to the the users to update their code. Also, the date in the comment is arbitrary, there is no real reason behind it. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By renaming the field from
That will no longer work as the user would need to do one of the following instead now:
or
IOW, I don't think the user can continue to just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fortunately that's not the case. In short, the
The only difference is, when calling the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aha! I didn't notice the |
||
def __str__(self) -> str: | ||
msg = 'Error: ' + str(self.message) + ', Code: ' + str(self.code) | ||
msg = 'Error: ' + str(self.message) + ', Status code: ' + str(self.status_code) | ||
if self.global_transaction_id is not None: | ||
msg += ' , X-global-transaction-id: ' + str(self.global_transaction_id) | ||
return msg | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
# coding=utf-8 | ||
# pylint: disable=missing-docstring,protected-access,too-few-public-methods | ||
# pylint: disable=missing-docstring,protected-access,too-few-public-methods,too-many-lines | ||
import gzip | ||
import json | ||
import os | ||
|
@@ -357,6 +357,7 @@ def test_request_server_error(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 500 | ||
assert err.value.status_code == 500 | ||
assert err.value.http_response.headers['Content-Type'] == 'application/json' | ||
assert err.value.message == 'internal server error' | ||
|
||
|
@@ -399,6 +400,7 @@ def test_request_success_invalid_json(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 200 | ||
assert err.value.status_code == 200 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to have a testcase that still uses the old way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, I'll add some! |
||
assert err.value.http_response.headers['Content-Type'] == 'application/json; charset=utf8' | ||
assert isinstance(err.value.__cause__, requests.exceptions.JSONDecodeError) | ||
assert "Expecting ':' delimiter: line 1" in str(err.value.__cause__) | ||
|
@@ -453,6 +455,7 @@ def test_request_fail_401_nonerror_json(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 401 | ||
assert err.value.status_code == 401 | ||
assert err.value.http_response.headers['Content-Type'] == 'application/json' | ||
assert err.value.message == error_msg | ||
|
||
|
@@ -473,6 +476,7 @@ def test_request_fail_401_error_json(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 401 | ||
assert err.value.status_code == 401 | ||
assert err.value.http_response.headers['Content-Type'] == 'application/json' | ||
assert err.value.message == error_msg | ||
|
||
|
@@ -492,6 +496,7 @@ def test_request_fail_401_nonjson(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 401 | ||
assert err.value.status_code == 401 | ||
assert err.value.http_response.headers['Content-Type'] == 'text/plain' | ||
assert err.value.message == response_body | ||
|
||
|
@@ -514,6 +519,7 @@ def test_request_fail_401_badjson(): | |
prepped = service.prepare_request('GET', url='') | ||
service.send(prepped) | ||
assert err.value.code == 401 | ||
assert err.value.status_code == 401 | ||
assert err.value.http_response.headers['Content-Type'] == 'application/json' | ||
assert err.value.message == response_body | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the same name in the
DetailedResponse
and I think it's a bit more clear than the simplecode
.