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

Comments should have no significance to the semantic meaning #69

Closed
almazik opened this issue Apr 10, 2020 · 7 comments
Closed

Comments should have no significance to the semantic meaning #69

almazik opened this issue Apr 10, 2020 · 7 comments
Labels
enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification

Comments

@almazik
Copy link

almazik commented Apr 10, 2020

Current parser tries to associate comments with the entities by assuming that the comment preceding the definition belongs to that definition.

According to the specification draft:

Comments are Ignored like white space and may appear after any token, or before a LineTerminator, and have no significance to the semantic meaning of a GraphQL Document.

So we should interpret comments just like the whitespace, skipping them during lex parsing.

This issue relates to #33 as documentation should replace the current comments annotation logic

@sungam3r sungam3r added the enhancement New feature or request label Apr 10, 2020
@sungam3r
Copy link
Member

sungam3r commented Apr 10, 2020

I just have to note that this is just a draft specification requirement. So for now, support for this feature can wait a bit.

@almazik
Copy link
Author

almazik commented Apr 10, 2020

Agree that it may wait, but for example HotChocolate and all graphql syntax highlighting plugins I've used already support it.

I've started implementing #33, as we might have to just switch to HotChocolate otherwise.

As a side note (which may be extracted into a separate issue), the current parsing logic (not the entities) doesn't seem to match the structure of the spec. I don't know if it's due to the historical development reasons, or the GraphQL spec have evolved quite far, but I think it would be great to refactor the parser to match the structure of the specification. For example (as defined in the spec):
Document has a list of Definitions.
Definition is either ExecutableDefinition, TypeSystemDefinition or TypeSystemExtension.
ExecutableDefinition is either an OperationDefinition or FragmentDefinition.
etc.
And parse the input using exactly the same hierarchy. It will allow the code to match the spec 1:1, and future changes would be much easier to implement. What do you think? I can come up with the implementation POC if you think it's worthwhile

@sungam3r
Copy link
Member

Document has a list of Definitions.
Definition is either ExecutableDefinition, TypeSystemDefinition or TypeSystemExtension.
ExecutableDefinition is either an OperationDefinition or FragmentDefinition.
etc.

The structure of the parser document is the same, isn't it?

@Shane32
Copy link
Member

Shane32 commented Aug 16, 2020

The current 2018 spec says the same thing about comments, by the way, that they are treated as white space. This seems like an important fix

@sungam3r sungam3r added the spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification label Aug 17, 2020
@sungam3r
Copy link
Member

Fixed by #105 ?

@Shane32
Copy link
Member

Shane32 commented Jan 16, 2021

Yes (optional). Also see #104 and #80

@Shane32 Shane32 closed this as completed Jan 16, 2021
@Shane32
Copy link
Member

Shane32 commented Jan 16, 2021

Also see #101 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request spec compliance The change is intended to solve the problem of inconsistency with the official GraphQL specification
Projects
None yet
Development

No branches or pull requests

3 participants