Skip to content

Gql parser 0.4 #396

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
May 30, 2022
Merged

Gql parser 0.4 #396

merged 1 commit into from
May 30, 2022

Conversation

mathstuf
Copy link
Contributor

Cc: @samuela

@mathstuf mathstuf mentioned this pull request Nov 21, 2021
Closed
@mathstuf
Copy link
Contributor Author

Also needs this PR now: graphql-rust/graphql-parser#57

@mathstuf mathstuf requested a review from tomhoule November 21, 2021 20:21
@mathstuf mathstuf marked this pull request as draft November 21, 2021 20:22
mathstuf
mathstuf commented Nov 21, 2021
View reviewed changes
@tomhoule
Copy link
Member

The extra lifetime and type parameters to do the same thing are a bit of a shame, but it's worth upgrading, thanks for the hard work! The PR looks good 👍 Let's wait for the graphql-parser PR to be merged and merge this.

@mathstuf mathstuf changed the title Gql parser 0.3 Gql parser 0.4 Nov 25, 2021
@jmhain
Copy link

jmhain commented Feb 9, 2022

Is this PR close to being merged? There are a couple advisories for failure which graphql_parser 0.2 depends on.

@samuela
Copy link

samuela commented Feb 10, 2022

graphql-rust/graphql-parser#57 has been merged for a few months now, so this one should be ready to go but I don't know what the status is

@mathstuf mathstuf marked this pull request as ready for review February 28, 2022 18:07
@mathstuf
graphql_client_codegen: update to graphql_parser 0.4
d0bc344 
Requires 0.4.x so that `Document<'a, String>::into_static()` exists.

Future work can try and reduce the amount of `'static` used throughout
the API, but this at least gets 0.4 into a working state.
@mathstuf
Copy link
Contributor Author

Sorry for the delay; I've rebased and fix the compilation error I had locally.

@mathstuf
Copy link
Contributor Author

What happened to CI here?

@jk-gan
Copy link

jk-gan commented May 25, 2022

Hi, may I know what is the status of this PR?

@mathstuf
Copy link
Contributor Author

Waiting on reviewers/maintainers AFAIK.

tomhoule
tomhoule approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the extra generics over different string types, but since we depend on graphql parser, let's upgrade. Thanks for the PR!

@tomhoule tomhoule merged commit 64ad4e3 into graphql-rust:main May 30, 2022
@mathstuf mathstuf deleted the gql-parser-0.3 branch May 30, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuela samuela samuela left review comments

@tomhoule tomhoule tomhoule 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.
5 participants
@mathstuf @tomhoule @jmhain @samuela @jk-gan