- Notifications
You must be signed in to change notification settings - Fork673
refactor: Replacing http_requests return type hint#2435
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b6eedfc to550263aCompareUh oh!
There was an error while loading.Please reload this page.
41adce2 toc7d5b71CompareUh oh!
There was an error while loading.Please reload this page.
The purpose of this change is to track API changes described inhttps://github.com/python-gitlab/python-gitlab/blob/main/docs/api-levels.rst,for example, for package versioning and breaking change announcementsin case of protocol changes.This is MVP implementation to be used by#2435.
/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. |
940a7b7 to7358573Compare/rerun-all |
codecov-commenter commentedFeb 17, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
📣 This organization is not using Codecov’sGitHub 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 toa712ddaCompare@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 ahttp_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 keepinghttp_backend on the client, while having backend-specific implementations in the backends package?
This way it's quite consistent with the existinghttp_* 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 toc264a12Compare
Uh oh!
There was an error while loading.Please reload this page.
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.