Codecov Report

Merging#2336 (9626499) intomain (9d2b1ad) willdecrease coverage by0.81%.
The diff coverage is84.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 inhttps://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 inhttps://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 inpython-redmine. The question I have with this approach is wether it goes along with our approach of eventually dropping therequest http client/engine and stay with onlyhttpx.

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 publichttp_* 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 havingAny 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 useAny.

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