-
Notifications
You must be signed in to change notification settings - Fork 18
Add support for graphql-core 3.2.0 #29
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
Conversation
Bump graphene minimum pin to 3.1 Only run push trigger on master branch Remove poetry.lock ref https://stackoverflow.com/a/61076546/5511061
@erikwrede if there's more you spotted, plz let me know :) CI is passing on my fork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
pyproject.toml
Outdated
@@ -16,7 +16,7 @@ classifiers = [ | |||
|
|||
[tool.poetry.dependencies] | |||
python = "^3.7" | |||
graphene = ">=3.0b6" | |||
graphene = "~=3.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the changes are possible without dropping support for graphene 3.0. GQLFormattedError
should already exist in the former GQL-Core release - will check tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like GQLFormattedError
was introduced in graphgl-core 3.2.0 in the same commit graphql-python/graphql-core@09ff14f (from diff graphql-python/graphql-core@v3.1.7...v3.2.0)
Since graphene 3.1 is the first release to (properly) support graphgl-core 3.2.0, I think this lower pin makes sense. Previous versions of this lib can still be used with graphql-core<3.2, risking dephell with the earlier versions of graphene/gql/graphql-core/graphql-relay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a closer look: graphene 3.1 still allows graphql-core 3.1, whereas this PR drops support. so either this PR should introduce compat import statements, or this PR should drop support:
graphene = "~=3.1" | |
graphene = "~=3.1" | |
graphql-core = "~=3.2" |
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestion seems to be necessary to prevent compatibility issues. Whether or not to introduce compatibility import statements is a question of scope. This is a quick fix, compatibility imports would increase complexity.
The cleaner way to go would be by supporting all core versions that graphene supports, aligning with the main library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
towards preventing future compatibility issues: ddelange#2
I think it's one step forward, plz let me know what you think :)
Synchronize graphql-core support with graphene
@ciscorn if you get the chance, please squash this PR as it turned out to be more than one commit 😅 |
thank you! |
Add minor pin on
graphql-core
as their minors are synchronized with upstream majors (docs)Only run push trigger on master branch
Remove poetry.lock ref https://stackoverflow.com/a/61076546/5511061
Fixes #26