- Notifications
You must be signed in to change notification settings - Fork673
chore: makeGitlab.http_request() a private method#1842
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
Convert `Gitlab.http_request()` to `Gitlab._http_request()` to signifyit is a private/protected method so that users of the library knowthey should not use the method and we make no API stability promisesfor using it.Add a `Gitlab.http_request()` method which will issue a Deprecationwarning when called. It will pass the call onto`Gitlab._http_request()`Also, in the interest of improving code read-ability, require keywordarg usage when calling `Gitlab._http_request()`
codecov-commenter commentedJan 15, 2022
Codecov Report
@@ Coverage Diff @@## main #1842 +/- ##======================================= Coverage 92.26% 92.27% ======================================= Files 77 77 Lines 4849 4853 +4 =======================================+ Hits 4474 4478 +4 Misses 375 375
Flags with carried forward coverage won't be shown.Click here to find out more.
|
| warnings.warn( | ||
| "The Gitlab.http_request() method is deprecated and will be removed in a " | ||
| "future version. This is a private method and should not be used.", | ||
| 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.
One thing about this method is that it provides a way for users to do stuff we haven't implemented (or perhaps even won't, if it's super specific). So I would maybe not be so strict about "should not be used". I would maybe say that API stability can't be guaranteed or something similar.
Edit: I've now checked a bit, because I was thinking the same that we should hide a bit more of the interface. But when looking at the original author's initial implementation, I think this might have been a deliberate choice and the distinction was more of a higher vs. lower-level API. At least what I can see from the commit message and how it was done with other private methods that were not meant to be consumed, see:c5ad540.
Multiple goals:
- Support making direct queries to the Gitlab server, without objects
and managers.
So I've started working on some docs to cover use cases here:#1846
It's a rough draft just for discussion on this, the wording and terminology is probably off and obviously "high/mid/low" can be changed to include "internal" or so.. but I think at least we should not take something away completely, unless we can cover those use cases in other ways. :)
Some use cases I vaguely remember from issues include using HEAD to check file existence/size, getting headers from the server's response, and so on. I documented some of that.
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 agree with@nejch here. It's also useful for new functionality in the GitLab API.
So let's tone the message down a bit.
Convert
Gitlab.http_request()toGitlab._http_request()to signifyit is a private/protected method so that users of the library know
they should not use the method and we make no API stability promises
for using it.
Add a
Gitlab.http_request()method which will issue a Deprecationwarning when called. It will pass the call onto
Gitlab._http_request()Also, in the interest of improving code read-ability, require keyword
arg usage when calling
Gitlab._http_request()