Codecov Report

Merging #2336 (9626499) into main (9d2b1ad) will decrease coverage by 0.81%.
The diff coverage is 84.42%.

@@            Coverage Diff             @@
##             main    #2336      +/-   ##
==========================================
- Coverage   95.96%   95.14%   -0.82%     
==========================================
  Files          80       82       +2     
  Lines        5331     5440     +109     
==========================================
+ Hits         5116     5176      +60     
- Misses        215      264      +49     

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

@lmilbaum Thanks for all the work on this!!
I just did a quick review. If you disagree with my comments don't be afraid to say so slightly_smiling_face I am often wrong!
One concern I have is all the adding of Any to a lot of the type-hints.

Yeah. I can understand why. I didn't find any other solution. The understanding that this is an interim step, helps me go for this option.

Thanks both for driving this, sorry again this is taking a while for me to get back into, I thought this would be easier and only the most low-level stuff factored out, similar to how it's done in https://python-redmine.com/advanced/request_engines.html / https://github.com/maxtepkeev/python-redmine/tree/master/redminelib/engines. So just wanted to make sure we take a good look before we merge if there's a simple way :)

@lmilbaum Thanks for all the work on this!!
I just did a quick review. If you disagree with my comments don't be afraid to say so slightly_smiling_face I am often wrong!
One concern I have is all the adding of Any to a lot of the type-hints.

Yeah. I can understand why. I didn't find any other solution. The understanding that this is an interim step, helps me go for this option.

Thanks both for driving this, sorry again this is taking a while for me to get back into, I thought this would be easier and only the most low-level stuff factored out, similar to how it's done in https://python-redmine.com/advanced/request_engines.html / https://github.com/maxtepkeev/python-redmine/tree/master/redminelib/engines. So just wanted to make sure we take a good look before we merge if there's a simple way :)

I liked the approach taken in python-redmine. The question I have with this approach is wether it goes along with our approach of eventually dropping the request http client/engine and stay with only httpx.

Would breaking this PR into smaller pieces would make sense?

Would breaking this PR into smaller pieces would make sense?

I think this might help yes @lmilbaum!
It would probably be a smaller PR if we don't take out all the methods as @JohnVillalovos suggested:

https://github.com/python-gitlab/python-gitlab/pull/2336/files#r1010073025
https://github.com/python-gitlab/python-gitlab/pull/2336/files#r1010073368

Also, it would potentially be a smaller PR if we ensure we keep public http_* methods in the main client so that we don't need to change the tests, because this is a public API on the Gitlab instance. We may just have private methods in backend clients that these wrap, but we somehow need to support this for now IMO.

https://python-gitlab.readthedocs.io/en/stable/api-levels.html

So biggest issue with this for me is the type-hints. Everything having Any added. I worked a LONG time adding type-hints to the code and this kind of throws it all away :(

So I think we need to figure out a solution that doesn't use Any.

Thank you for the feedback @nejch and @JohnVillalovos. I've started breaking this up into smaller PRs. Take a look at #2382