Allow to parse more AST nodes #306
Conversation
github-actions
bot
added
the
test
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
| } | ||
|
|
||
| internal GraphQLValue ParseValueLiteral(bool isConstant) | ||
| public GraphQLValue ParseValueLiteral(bool? isConstant) |
There was a problem hiding this comment.
I don’t understand why these were changed to nullable booleans or what null does compared to false. Here I don’t see a difference.
There was a problem hiding this comment.
GraphQLVariable inherits GraphQLValue and I called ParseValueLiteral with true before. In case of GraphQLVariable this method throws. null as a value of isConstant means that caller wants to parse any value - either constant or not (variable).
There was a problem hiding this comment.
Core difference is here:
variable will be parsed for false AND null
There was a problem hiding this comment.
I still fail to understand why it's not a bool instead of bool?. It seems that null behaves identically to false. Why the change?
There was a problem hiding this comment.
Yep, it works without ?, reverted.
src/GraphQLParser/Parser.cs
Outdated
|
|
||
| else if (typeof(T) == typeof(GraphQLValue)) | ||
| result = (T)(object)context.ParseValueLiteral(true); | ||
| result = (T)(object)context.ParseValueLiteral(null); |
There was a problem hiding this comment.
And here it’s now null. I’m sorta puzzled. Certainly for your use you would need the ability to have this as false, right ? And if false is null then why not false?
There was a problem hiding this comment.
For my use case I wanted to be able to parse literals, so I wrote true here. Now I added support for parsing variables. Variable inherits GraphQLValue but parser will throw if you call ParseValueLiteral(true) on text $id.
There was a problem hiding this comment.
I did not change calls inside parser - they all continue to pass either true or false as before. The single place where I pass null is here.
There was a problem hiding this comment.
Are you adding something to the API so you can call ParseValueLiteral(true)? Or are you going to write an AST visitor to ensure that the value does not reference a variable?
There was a problem hiding this comment.
I reverted bool? to bool and now pass false. It works. My bad.
|
@Shane32 Are you OK to release v9 after merge? |
Sure |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #306 +/- ##
==========================================
+ Coverage 96.02% 96.23% +0.20%
==========================================
Files 85 85
Lines 4835 4889 +54
Branches 448 475 +27
==========================================
+ Hits 4643 4705 +62
+ Misses 190 184 -6
+ Partials 2 0 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fixes #296