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
refactor: Replacing http_requests return type hint #2435
Conversation
b6eedfc
to
550263a
Compare
f903c7a
to
9a1ba0b
Compare
41adce2
to
c7d5b71
Compare
The purpose of this change is to track API changes described in https://github.com/python-gitlab/python-gitlab/blob/main/docs/api-levels.rst, for example, for package versioning and breaking change announcements in case of protocol changes. This is MVP implementation to be used by #2435.
c7d5b71
to
e515df7
Compare
/rerun-all |
@nejch after moving the protocols to the backend, this change will pass because the change is for the front end (client) http_request call. The whole point of the protocol is to identify this kind of change. |
e515df7
to
5aff2f4
Compare
940a7b7
to
7358573
Compare
/rerun-all |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2435 +/- ##
=======================================
Coverage 96.15% 96.15%
=======================================
Files 87 87
Lines 5663 5667 +4
=======================================
+ Hits 5445 5449 +4
Misses 218 218
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7476d56
to
a712dda
Compare
@nejch @JohnVillalovos Can you please review this PR? |
utils.warn( | ||
"`http_request()` is deprecated and will be removed in a future version.\n" | ||
"Please use `backend_request()` instead.", | ||
category=DeprecationWarning, | ||
) |
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.
I'm a bit wary of this, it would feel wrong deprecating this after we already had a similar discussion in #1842. What would be the issue with having a http_request
in the public client?
Keeping in mind that the user does not care about the backends generally.
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.
This is a breaking change, so this would need to be at least communicated to users via a breaking change trailer (and triger 4.0.0).
I've thought it would make sense using this method and not to trigger 4.0.0 for now.
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.
@lmilbaum what I meant was what would be wrong with simply keeping http_backend
on the client, while having backend-specific implementations in the backends package?
This way it's quite consistent with the existing http_*
methods (there is also some history in the other MR, where we discussed we'd keep it in the public client as it is).
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.
I am not following. http_backend
is kept on the client while having backend-specific implementation in the backend package.
I was just trying to address the valid concern you have raised regarding the breaking change.
5be9523
to
c264a12
Compare
http_request() will be deprecated in future release in favor of backend_request(). That is because of the return type change and the need to warn users of the change.