fix: more consistent naming of HTTP status code related attributes #185
Conversation
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/fix-retry-and-naming
branch
from
2d0f9dd to
ccc5a9e
Compare
pyrooka
changed the title
Retry attribute and more consistent naming
| @@ -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 | |||
There was a problem hiding this comment.
We use the same name in the DetailedResponse and I think it's a bit more clear than the simple code.
| # 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 | ||
|
|
There was a problem hiding this comment.
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.
By renaming the field from code to status_code above, I think you've introduced a breaking change despite the addition of this new getter function.
What I mean is... previously a user would need to access the code field like this:
print('The status code is: ', exception.code)
That will no longer work as the user would need to do one of the following instead now:
print('The status code is: ', exception.status_code)
or
print('The status code is: ', exception.code())
IOW, I don't think the user can continue to just use exception.code, can they???
There was a problem hiding this comment.
Fortunately that's not the case. In short, the @property decorator creates a valid attribute (well only with a getter by default), so it can be used as before. See the example below;
>>> ex = ApiException(404, message="Not found...")
>>> ex.code
404
>>> ex.status_code
404
The only difference is, when calling the code property, a deprecation message will be printed to the stderr at warning level.
There was a problem hiding this comment.
Aha! I didn't notice the @property decorator, and I probably wouldn't have known what that was for even if I did 😂.
| # 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 | ||
|
|
There was a problem hiding this comment.
By renaming the field from code to status_code above, I think you've introduced a breaking change despite the addition of this new getter function.
What I mean is... previously a user would need to access the code field like this:
print('The status code is: ', exception.code)
That will no longer work as the user would need to do one of the following instead now:
print('The status code is: ', exception.status_code)
or
print('The status code is: ', exception.code())
IOW, I don't think the user can continue to just use exception.code, can they???
| @@ -398,7 +398,7 @@ def test_request_success_invalid_json(): | |||
| service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator()) | |||
| 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.
I think it would be good to have a testcase that still uses the old way err.value.code to make sure that still works (see my previous comment about this above)
There was a problem hiding this comment.
Good idea, I'll add some!
Signed-off-by: Norbert Biczo <[email protected]>
pyrooka
force-pushed
the
nb/fix-retry-and-naming
branch
from
f249880 to
09be05d
Compare
# [3.19.0](v3.18.2...v3.19.0) (2024-01-22) ### Features * rename `ApiException.code` to `ApiException.status_code` ([#185](#185)) ([cf74671](cf74671))
|
🎉 This PR is included in version 3.19.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This commit fixes a small inconsistency in the naming
of the HTTP status code attributes between the successful
and the error responses.