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

Make parsing publicly accessible #776

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mirosval
Copy link

@mirosval mirosval commented Oct 6, 2020

Partial fix for #726
Reference #773

I've added parse as a public method on the GraphQLRequest type. I've documented and exposed Document, Spanning and ParseError to allow working with the resulting type.

Adds a parse method on GraphQLRequest
Exposes and documents types that make that possible: Document, Spanning and ParseError
adds a very simple example that does not use any frameworks and just demonstrates how the library works on its own
@ccbrown
Copy link
Contributor

ccbrown commented Oct 6, 2020

I think we could kill two birds with one stone if instead of adding a parse method to GraphQLRequest, we just added a method to get the query. This would address part of #750.

I think it's also worth it to look into eliminating the schema / scalar template parameter requirement from parsing before making Document public so that we don't have to make a backwards-incompatible change later. If you'd like, I can probably take some time to investigate this further.

@mirosval
Copy link
Author

mirosval commented Oct 7, 2020

Thanks @ccbrown! I'm not sure I understand where you're suggesting to add the method. Are you suggesting that:

  1. We move the parse method elsewhere (as a free fn) AND
  2. Add a simple accessor method to get the query out of GraphQLRequest?

Regarding the Scalar in Document, how about I create another PR, address that and then we can come back to this one and revise it?

@ccbrown
Copy link
Contributor

ccbrown commented Oct 7, 2020

Are you suggesting that:

  1. We move the parse method elsewhere (as a free fn) AND
  2. Add a simple accessor method to get the query out of GraphQLRequest?

Just 2. Then delete the parse method because we could just use the existing parse_document_source function with GraphQLRequest.

Regarding the Scalar in Document, how about I create another PR, address that and then we can come back to this one and revise it?

Sounds good to me. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants