Rework of all value nodes + tune error messages #166
Conversation
github-actions
bot
added
the
test
|
I plan to add tests in further PRs to increase code coverage. Also I think about switching to file-scoped namespaces. There are no PRs in this repo so no conflicts. |
sungam3r
added
enhancement
| /// <see cref="ASTNodeKind.NullValue">Null</see> | ||
| /// </summary> | ||
| [DebuggerDisplay("{Value}")] | ||
| public class GraphQLScalarValue : GraphQLValue |
There was a problem hiding this comment.
Bad design here, misleading naming. I removed this node. Also now all child nodes don't store ASTNodeKind -> less memory.
| [InlineData("enum E { true A }", "Unexpected Name \"true\"; enum values are represented as unquoted names but not 'true' or 'false' or 'null'.", 1, 10)] | ||
| [InlineData("enum E { B false }", "Unexpected Name \"false\"; enum values are represented as unquoted names but not 'true' or 'false' or 'null'.", 1, 12)] | ||
| [InlineData("enum E { A null B }", "Unexpected Name \"null\"; enum values are represented as unquoted names but not 'true' or 'false' or 'null'.", 1, 12)] | ||
| public void Should_Throw_On_Invalid_EnumValue(string query, string description, int line, int column) |
There was a problem hiding this comment.
Test failed before changes since EnumValueDefinition accepts true/false/null that contradicts spec.
There was a problem hiding this comment.
I'll have to see the corresponding changes required in GraphQL.NET to really know how these changes will work. I suggest having the dev branch pull in the beta build from GitHub Packages before publishing the new version here. Here's a few concerns I see:
- Is there a unified way of retrieving the Value of a node -- or certainly a scalar node -- without having to cast it to it's specific node type? Does it matter?
- How does elimination of GraphQLScalarValue from the inheritance chain affect use?
|
I think that nothing bad will happen. See |
|
I would say that the parser object model / API evolved to such an extent that ready to "absorb" |
|
Again, let's use the beta build in graphql.net before releasing vNext of parser, so we can make tweaks if necessary |
|
Of course. I was not going to release the parser separately. |
|
I still have work in the parser, a few PRs. When done I'll bump parser version on |
I continue to work on strict grammar.