Skip to content

Conversation

@nejch
Copy link
Member

@nejch nejch commented Nov 25, 2024

Users have been asking for years for some kind of async support. This is pretty easy to do with the GraphQL client as we're only talking to a single endpoint so it's a good start.

Changes

Adds AsyncGraphQL and a private base client for common graphql logic.

Related to #1357
Related to #1025

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

@codecov
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.64%. Comparing base (8046387) to head (ad75cab).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3045      +/-   ##
==========================================
+ Coverage   96.62%   96.64%   +0.01%     
==========================================
  Files          95       95              
  Lines        6106     6142      +36     
==========================================
+ Hits         5900     5936      +36     
  Misses        206      206              
Flag Coverage Δ
api_func_v4 82.46% <83.67%> (-0.03%) ⬇️
cli_func_v4 82.59% <34.69%> (-0.31%) ⬇️
unit 88.83% <97.95%> (+0.06%) ⬆️

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

Files with missing lines Coverage Δ
gitlab/__init__.py 100.00% <100.00%> (ø)
gitlab/_backends/graphql.py 100.00% <100.00%> (ø)
gitlab/client.py 98.72% <100.00%> (+0.06%) ⬆️

@nejch
Copy link
Member Author

nejch commented Nov 25, 2024

@JohnVillalovos @max-wittig this should be ready now. I opted for two separate clients rather than a async=True flag as it seemed a bit more explicit with typing and less of a pain. Otherwise, seems like a quick win as we can reuse gql + httpx quite a bit.

The diffs are super weird on GitHub, might be easier to check locally.

gitlab/client.py Outdated


class GraphQL:
class _BaseGrapQL:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class _BaseGrapQL:
class _BaseGraphQL:

gitlab/client.py Outdated

opts = self._get_client_opts()
self._http_client = client or httpx.Client(**opts)
class GraphQL(_BaseGrapQL):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class GraphQL(_BaseGrapQL):
class GraphQL(_BaseGraphQL):

gitlab/client.py Outdated
return result


class AsyncGraphQL(_BaseGrapQL):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class AsyncGraphQL(_BaseGrapQL):
class AsyncGraphQL(_BaseGraphQL):



@pytest.fixture(scope="module")
def api_url():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def api_url():
def api_url() -> str:

Copy link
Member

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Let's fix some typos

@nejch
Copy link
Member Author

nejch commented Dec 3, 2024

Thanks @max-wittig should be ready now 🙇

@nejch nejch force-pushed the feat/async-graphql branch from 6b5632f to 0307df3 Compare December 3, 2024 15:06
@max-wittig max-wittig enabled auto-merge (rebase) December 4, 2024 11:11
@nejch nejch force-pushed the feat/async-graphql branch from 0307df3 to ad75cab Compare December 4, 2024 13:11
@max-wittig max-wittig merged commit 288f39c into main Dec 4, 2024
16 checks passed
@max-wittig max-wittig deleted the feat/async-graphql branch December 4, 2024 13:28
@roma-glushko
Copy link

Hey folks, I have installed v5.1.0 and could not find the async client, so it must be not released yet.
When you do plan to include it in the next version of the client?

@nejch
Copy link
Member Author

nejch commented Dec 17, 2024

Hi @roma-glushko the next release is auto-scheduled for the 28th. If there's demand though we can trigger it before.

@roma-glushko
Copy link

@nejch I would love to give it a try, so if it's not too much trouble to release sooner I would really appreciate that (at least a prerelease version if the pipeline you have supports that).

Otherwise, I can try to temporary pull it from the main branch.

@nejch
Copy link
Member Author

nejch commented Dec 17, 2024

This is now released. There's a bit of blocking code in there for the retry logic AFAIK, so let us know if it doesn't work as expected.

@roma-glushko
Copy link

roma-glushko commented Dec 18, 2024

@nejch Hey Nejc, this is awesome! Thank you for releasing it soo fast (it's version 5.2.0 in case anyone else is curious).
Gonna give it a try and let you know if there is anything.

There's a bit of blocking code in there for the retry logic AFAIK, so let us know if it doesn't work as expected.

Okay, that's good to know 👁️ I think this is fine for now. Do you guys have any plans to address that part?

@nejch
Copy link
Member Author

nejch commented Dec 18, 2024

@roma-glushko for sure if we get any feedback from users as you most likely have more uses for this than ourselves at the moment :) It was mostly so we could reuse some sync/async client code.

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.

4 participants