NotificationsYou must be signed in to change notification settings
Fork673
Star2.4k

feat(client): bootstrap the http backends concept#2391

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

Jump to bottom

Merged

nejch merged 1 commit intopython-gitlab:mainfromlmilbaum:http_engines

Dec 5, 2022

Merged

feat(client): bootstrap the http backends concept#2391

nejch merged 1 commit intopython-gitlab:mainfromlmilbaum:http_engines

Dec 5, 2022

Conversation

Copy link

lmilbaum commentedNov 24, 2022•
edited
Loading

Initial implementation of http engine concept. The long-term target is to shift all requests/response processing from the client to the http engine. When that is done for requests then it would be easy enough to add another engine, like httpx.

Copy link

codecov-commenter commentedNov 24, 2022•
edited
Loading

Codecov Report

Merging#2391 (c9888ef) intomain (faf842e) willincrease coverage by0.00%.
The diff coverage is100.00%.

@@           Coverage Diff           @@##             main    #2391   +/-   ##=======================================  Coverage   95.98%   95.99%           =======================================  Files          80       82    +2       Lines        5360     5372   +12     =======================================+ Hits         5145     5157   +12  Misses        215      215

Flag	Coverage Δ
api_func_v4	`83.80% <100.00%> (+0.03%)`	⬆️
cli_func_v4	`82.46% <100.00%> (+0.03%)`	⬆️
unit	`87.82% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
gitlab/client.py	`98.55% <100.00%> (+<0.01%)`	⬆️
gitlab/http_backends/__init__.py	`100.00% <100.00%> (ø)`
gitlab/http_backends/requests_backend.py	`100.00% <100.00%> (ø)`

lmilbaum force-pushed thehttp_engines branch from2717a1b to07aba4fCompare

November 24, 2022 02:52

lmilbaum marked this pull request as ready for review

November 24, 2022 03:04

lmilbaum force-pushed thehttp_engines branch from07aba4f toe043182Compare

November 24, 2022 03:19

lmilbaum mentioned this pull request

Nov 24, 2022

feat: allow passing kwargs to Gitlab class when instantiating withfrom_config#2392

Merged

nejch reviewed

Nov 26, 2022

View reviewed changes

Copy link

Member

nejch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for breaking this up@lmilbaum! Just a few minor questions.

We used the redminelib approach here IIRC, after poking around github a bit more I think also calling thesehttp backends, e.g.from gitlab import backends might make sense, but just a thought.

gitlab/client.py OutdatedShow resolvedHide resolved

gitlab/http_engines/requests_engine.py OutdatedShow resolvedHide resolved

gitlab/client.py OutdatedShow resolvedHide resolved

lmilbaum force-pushed thehttp_engines branch 5 times, most recently from1cbb77c toba2ee6cCompare

November 26, 2022 17:14

lmilbaum requested a review fromnejch

November 26, 2022 17:28

Copy link

Author

lmilbaum commentedNov 26, 2022

I would prefer that#2392 lands before this one. It is needed for testing this PR.

Copy link

Member

nejch commentedNov 26, 2022•
edited
Loading

I would prefer that#2392 lands before this one. It is needed for testing this PR.

@lmilbaum it shouldn't block if you cherry-pick the commit from#2397 here (feel free to cherry pick already), it seems like the tests in that PR are really meant for this one, then we can really test the method we are changing (__init__) and not indirectly through another method.

(Our use ofGitlab.from_config() in tests is a bit odd, leading to problems like this one. I think we should move away from it for default tests. Then we can look atfrom_config() independently :) )

Edit: maybe I was a bit quick with that PR and didnt check the tests ;) but in any case on second thought I think the tests can be done in unit tests as we don't make requests against a real server in those. So shouldn't block this PR IMO.