Skip to content
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

Add sync batching to requests sync transport #431

Merged
merged 15 commits into from
Sep 5, 2023
Merged

Add sync batching to requests sync transport #431

merged 15 commits into from
Sep 5, 2023

Conversation

itolosa
Copy link
Contributor

@itolosa itolosa commented Sep 4, 2023

Feature

Enable batching.

It adds execute_batch method for Transport and Client in order to allow users to send multiple requests at once.

Server must allow this type of requests.

Issue: #430

Resources:

@itolosa itolosa marked this pull request as draft September 4, 2023 20:33
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (013fa6a) 100.00% compared to head (1e4c981) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##            master      #431    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           27        28     +1     
  Lines         2388      2492   +104     
==========================================
+ Hits          2388      2492   +104     
Files Changed Coverage Δ
gql/transport/aiohttp.py 100.00% <ø> (ø)
gql/__init__.py 100.00% <100.00%> (ø)
gql/client.py 100.00% <100.00%> (ø)
gql/graphql_request.py 100.00% <100.00%> (ø)
gql/transport/requests.py 100.00% <100.00%> (ø)
gql/transport/transport.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@itolosa
Copy link
Contributor Author

itolosa commented Sep 4, 2023

No clue why some tests are failing because of some missing aiohttp. It just happens on the CI

Edited: Because of some missing test marks

@leszekhanusz
Copy link
Collaborator

No clue why some tests are failing because of some missing aiohttp. It just happens on the CI

In the CI, we are testing that some tests are supposed to work only with a single transport dependency installed.

If you run pytest tests --requests-only, it should work even if aiohttp is not installed (after running pip uninstall aiohttp)

It is possible to mark the tests to specify which test can be run in which condition. But I just see that you found out about it just now.

Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

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

This look good already!
Also nice to see that the countries.trevorblades.com site we use for our tests supports batch requests.
I've noted a few requested changes.
If you could also please check again all the docstrings so that the parameters correspond to the new ones and the text is modified to say that it's a batch operation.
Thanks!

docs/code_examples/console_async.py Outdated Show resolved Hide resolved
gql/client.py Outdated Show resolved Hide resolved
gql/transport/data_structures/graphql_request.py Outdated Show resolved Hide resolved
gql/transport/httpx.py Outdated Show resolved Hide resolved
gql/transport/requests.py Outdated Show resolved Hide resolved
gql/transport/transport.py Outdated Show resolved Hide resolved
gql/transport/transport.py Outdated Show resolved Hide resolved
tests/custom_scalars/test_money.py Outdated Show resolved Hide resolved
gql/client.py Outdated Show resolved Hide resolved
tests/test_httpx.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@itolosa itolosa left a comment

Choose a reason for hiding this comment

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

Pushed some fixes

Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

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

a few nitpicking changes.

gql/client.py Outdated Show resolved Hide resolved
gql/client.py Outdated Show resolved Hide resolved
gql/transport/requests.py Outdated Show resolved Hide resolved
gql/transport/requests.py Outdated Show resolved Hide resolved
gql/transport/requests.py Outdated Show resolved Hide resolved
gql/transport/transport.py Outdated Show resolved Hide resolved
tests/custom_scalars/test_money.py Outdated Show resolved Hide resolved
tests/test_client.py Outdated Show resolved Hide resolved
tests/test_graphql_request.py Outdated Show resolved Hide resolved
tests/test_transport_batch.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@leszekhanusz leszekhanusz left a comment

Choose a reason for hiding this comment

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

LGTM

@leszekhanusz leszekhanusz marked this pull request as ready for review September 5, 2023 19:08
@leszekhanusz leszekhanusz merged commit d4c9751 into graphql-python:master Sep 5, 2023
15 checks passed
@itolosa
Copy link
Contributor Author

itolosa commented Sep 5, 2023

Nice work! 😄

@leszekhanusz
Copy link
Collaborator

Thanks for your help!

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.

None yet

2 participants