refactor: Replacing http_requests return type hint#2435
refactor: Replacing http_requests return type hint#2435lmilbaum wants to merge 1 commit intopython-gitlab:mainfrom
Conversation
lmilbaum
force-pushed
the
http_request
branch
2 times, most recently
from
b6eedfc to
550263a
Compare
lmilbaum
force-pushed
the
http_request
branch
from
f903c7a to
9a1ba0b
Compare
lmilbaum
force-pushed
the
http_request
branch
2 times, most recently
from
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.
lmilbaum
force-pushed
the
http_request
branch
from
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. |
lmilbaum
force-pushed
the
http_request
branch
from
e515df7 to
5aff2f4
Compare
lmilbaum
force-pushed
the
http_request
branch
2 times, most recently
from
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.
|
lmilbaum
force-pushed
the
http_request
branch
2 times, most recently
from
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.
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.
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.
@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.
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.
lmilbaum
force-pushed
the
http_request
branch
2 times, most recently
from
5be9523 to
c264a12
Compare
3 participants
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.