Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 5, 2022
Merged

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

merged 1 commit into from
Dec 5, 2022

Conversation

lmilbaum
Copy link

@lmilbaum lmilbaum commented Nov 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.

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022 •
edited
Loading

Codecov Report

Merging #2391 (c9888ef) into main (faf842e) will increase coverage by 0.00%.
The diff coverage is 100.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 lmilbaum marked this pull request as ready for review November 24, 2022 03:04
@lmilbaum lmilbaum mentioned this pull request Nov 24, 2022
Merged
nejch
nejch reviewed Nov 26, 2022
View reviewed changes
Copy link
Member

@nejch 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 these http backends, e.g. from gitlab import backends might make sense, but just a thought.

@lmilbaum lmilbaum force-pushed the http_engines branch 5 times, most recently from 1cbb77c to ba2ee6c Compare November 26, 2022 17:14
@lmilbaum lmilbaum requested a review from nejch November 26, 2022 17:28
@lmilbaum
Copy link
Author

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

@nejch
Copy link
Member

nejch commented Nov 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 of Gitlab.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 at from_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.

@lmilbaum
Copy link
Author

lmilbaum commented Dec 2, 2022

@nejch could you please take a look. I would like to move forward. Thanks.

nejch
nejch requested changes Dec 4, 2022
View reviewed changes
Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

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

Sorry for the delays @lmilbaum, was out for a week. Looks great, just some tiny questions on the typing approach.

@lmilbaum lmilbaum force-pushed the http_engines branch 2 times, most recently from 24d5afa to da6bc2b Compare December 5, 2022 06:49
@lmilbaum lmilbaum requested a review from nejch December 5, 2022 07:07
@nejch nejch changed the title feat: Bootstrapping the http_engines concept feat(client): bootstrap the http backends concept Dec 5, 2022
nejch
nejch reviewed Dec 5, 2022
View reviewed changes
nejch
nejch approved these changes Dec 5, 2022
View reviewed changes
Copy link
Member

@nejch 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 again for all the work here @lmilbaum!

@nejch nejch merged commit 91a665f into python-gitlab:main Dec 5, 2022
@lmilbaum lmilbaum deleted the http_engines branch December 5, 2022 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nejch nejch nejch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lmilbaum @codecov-commenter @nejch