Skip to content

chore: make Gitlab.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

JohnVillalovos
Copy link
Member

Convert Gitlab.http_request() to Gitlab._http_request() to signify
it 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 Deprecation
warning 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()

Convert `Gitlab.http_request()` to `Gitlab._http_request()` to signify
it 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 Deprecation
warning 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()`
@JohnVillalovos JohnVillalovos marked this pull request as draft January 15, 2022 02:59
@codecov-commenter
Copy link

Codecov Report

Merging #1842 (baaba22) into main (a1dbe86) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1842   +/-   ##
=======================================
  Coverage   92.26%   92.27%           
=======================================
  Files          77       77           
  Lines        4849     4853    +4     
=======================================
+ Hits         4474     4478    +4     
  Misses        375      375           
Flag Coverage Δ
cli_func_v4 81.26% <77.77%> (-0.03%) ⬇️
py_func_v4 80.23% <77.77%> (-0.03%) ⬇️
unit 83.30% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
gitlab/client.py 90.64% <100.00%> (+0.09%) ⬆️
gitlab/mixins.py 91.50% <0.00%> (ø)
gitlab/v4/objects/files.py 100.00% <0.00%> (ø)

Comment on lines +622 to +625
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,
Copy link
Member

@nejch nejch Jan 16, 2022

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants