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

Allow to parse value literals #295

Merged
merged 1 commit into from
Apr 16, 2023
Merged

Allow to parse value literals #295

merged 1 commit into from
Apr 16, 2023

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Apr 16, 2023

I need to parse values returned from introspection request, mostly default values of arguments. Our current design allows to parse only entire GraphQL document.

Related project - https://github.com/sungam3r/graphql-introspection-model

@sungam3r sungam3r self-assigned this Apr 16, 2023
@sungam3r sungam3r requested a review from Shane32 April 16, 2023 21:36
@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Apr 16, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #295 (54e7615) into master (0211f3d) will not change coverage.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##            master      #295   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           85        85           
  Lines         4526      4532    +6     
  Branches       455       455           
=========================================
+ Hits          4526      4532    +6     
Impacted Files Coverage Δ
src/GraphQLParser/ParserContext.Parse.cs 100.00% <ø> (ø)
src/GraphQLParser/Parser.cs 100.00% <100.00%> (ø)
src/GraphQLParser/ParserContext.cs 100.00% <100.00%> (ø)
src/GraphQLParser/Visitors/SDLPrinter.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sungam3r
Copy link
Member Author

@Shane32 I'm rewriting https://github.com/sungam3r/graphql-introspection-model now. I decided to completely remove my own SDLBuilder class that prints schema and switch to SDLPrinter from parser project. To be able to do this I wrote ASTConverter that converts my introspection representations into AST representations of GraphQL-Parser. Doing this I noticed that I need to properly handle all literals returned as default values or arguments. Fortunately all code I need is already here, but not as public API.

@sungam3r sungam3r added the enhancement New feature or request label Apr 16, 2023
@@ -661,6 +661,7 @@ namespace GraphQLParser
public static class Parser
{
public static GraphQLParser.AST.GraphQLDocument Parse(GraphQLParser.ROM source, GraphQLParser.ParserOptions options = default) { }
public static GraphQLParser.AST.GraphQLValue ParseValue(GraphQLParser.ROM source, GraphQLParser.ParserOptions options = default) { }
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a note - it seems that we could make more methods public to allow parse concrete parts of document.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was thinking like "couldn't we just have Parse" return whatever was parsed? Then cast it to whatever type was desired or something? But the problem is that the parser wouldn't know the initial state -- if, for example, it was parsing a list of arguments or a list of directives.

We could perhaps add Parse<T> where T is a node type, and then a switch typeof(T) logic to bump into the correct code.

Copy link
Member

Choose a reason for hiding this comment

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

Then you could write Parse<GraphQLValue> if you want that, or Parse<List<GraphQLArgument>> if you are parsing that, etc, with very little API changes and somewhat intuitive behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Not really a fan of a million different public Parse... methods. Hopefully they are all exposed as protected methods or something so anyone CAN use them if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a subject to discuss for v9.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sungam3r sungam3r mentioned this pull request Apr 16, 2023
@sungam3r sungam3r merged commit fc193ad into master Apr 16, 2023
8 checks passed
@sungam3r sungam3r deleted the value branch April 16, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants