Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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. 👍

# for free to join this conversation on GitHub. Already have an account? # 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