Skip to content

Conversation

KingDarBoja
Copy link
Contributor

Based on this graphene PR and reading about GraphQL sample schema, I tried to replicate the subscription feature using gql library.

There were big issues as this isn't well documented but this PR should provide insight for end users to use GraphQL subscriptions along with gql.

The only issue is that asyncio, async/await features are not supported on Python 2.7.
Also, the yield keyword will throw a SyntaxError on Python 3.5.

Also, the Episode enumerable was using integers instead of strings values, so I replaced them to follow the sample GraphQL schema.

For last, this PR also provides support for passing GraphQLInputObjectType and GraphQLInputObjectField if using DSL based on #24.

Cheers!

@coveralls
Copy link

coveralls commented Mar 9, 2020

Coverage Status

Coverage decreased (-0.4%) to 98.618% when pulling 42aac91 on KingDarBoja:docs-subscriptions into fce1de3 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 98.5% when pulling 39e69c0 on KingDarBoja:docs-subscriptions into 523bb72 on graphql-python:master.

@KingDarBoja
Copy link
Contributor Author

@Cito Can you provide some feedback in this one? Thank you 😃

Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

You can put the tests that don't run in Python 2 in a separate directory like here.

Also, refactor the episode Enum to fit the spec.
Replace old mutation tests with a new one based on GraphQL spec examples.
@KingDarBoja KingDarBoja requested a review from Cito March 13, 2020 19:13
Copy link
Member

@Cito Cito left a comment

Choose a reason for hiding this comment

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

LGTM. Just two cosmetic issues.

@KingDarBoja KingDarBoja requested a review from Cito March 15, 2020 17:12
@KingDarBoja
Copy link
Contributor Author

Looks like mypy got mad about some definitions!

@Cito
Copy link
Member

Cito commented Mar 15, 2020

I merge this anyway and will fix the mypy issues then.

@Cito Cito merged commit b4e17f0 into graphql-python:master Mar 15, 2020
@Cito
Copy link
Member

Cito commented Mar 15, 2020

@KingDarBoja: Maybe you can make your next PRs and commits a bit more focused, i.e. better create more small PRs and commits than one large one, and keep the janitor work, cleanups etc. separate from the code changes and bug fixes.

@Cito
Copy link
Member

Cito commented Mar 15, 2020

The mypy issues are fixed now in 2aeffa5. I simply silenced most warnings, some were caused by incomplete type hints in graphql-core 2. But we should not waste too much time with this. The typing stuff is much better in graphql-core 3 where we can also use real Python 3 type hints.

@KingDarBoja KingDarBoja deleted the docs-subscriptions branch March 16, 2020 02:43
@KingDarBoja
Copy link
Contributor Author

@KingDarBoja: Maybe you can make your next PRs and commits a bit more focused, i.e. better create more small PRs and commits than one large one, and keep the janitor work, cleanups etc. separate from the code changes and bug fixes.

Thanks for the suggestion, I will keep that on mind. Small PRs to address a specific bug fix or feature while cleanup and all those stuff on another PR.

The mypy issues are fixed now in 2aeffa5. I simply silenced most warnings, some were caused by incomplete type hints in graphql-core 2. But we should not waste too much time with this. The typing stuff is much better in graphql-core 3 where we can also use real Python 3 type hints.

I was going to ask at the Slack channel about spending some extra time (at least for me) to go around graphql-core-legacy and fix some issues to clean the repo from staled issues and pull requests due to development of graphql-core-3 being the current focus.

Shall I do it or just ignore any related issue and instead wait for the gql-next migration plan?

@Cito
Copy link
Member

Cito commented Mar 22, 2020

Some people are waiting for issues and PR in graphql-core-legacy to be processed. If you can help with that, this will be good. I simply do not have the time.

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.

3 participants