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

Feature/add comment in ast #2471

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nveenjain
Copy link

Fixes #2241
Added support to add comments in lexer itself.
Modified the tests as the root structure of the response is changed.
Added some tests. Please comment if more test cases are required.

@nveenjain nveenjain force-pushed the feature/addCommentInAST branch 2 times, most recently from fc3ed54 to 0b877aa Compare March 3, 2020 06:05
@nveenjain nveenjain force-pushed the feature/addCommentInAST branch 3 times, most recently from d78a38d to e4cedca Compare March 5, 2020 12:31
@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Mar 6, 2020
@nveenjain
Copy link
Author

nveenjain commented Mar 7, 2020

Edit: Resolved the issue by abstracting advance in parser... Will update the PR soon.

@IvanGoncharov while migrating the logic of comment skipping to the parser, I noticed that we are using this._lexer.token to get the location for many nodes... This might cause an issue when called after this._lexer.advance()/this._lexer.lookahead()(through parser) it would mean that we would need to change the logic to skip comments at all the places where this._lexer.advance()/this._lexer.lookahead() is called in parser, since next token might be a comment... (which means lots of changes, and lots of tests to satisfy coverage check)

Is it desired, or should I follow some other approach towards moving skipping functionality to parser like creating lexer.lookahead in the parser itself and renaming lexer.lookahead function to lexer._lookahed or something else?

Reference:-
I added skipping functionality to peek, expectToken, expectKeyword, expectOptionalKeyword and expectOptionalToken but
parses type with description multi-line string failed because we are using this._lexer.lookahead() and checking whether keywordToken.kind === TokenKind.NAME where keywordToken is lookahead, it fails because keywordToken is comment token...

@nveenjain nveenjain force-pushed the feature/addCommentInAST branch 2 times, most recently from 19f2566 to 3da9a6a Compare March 7, 2020 13:14
@nveenjain nveenjain requested a review from IvanGoncharov March 7, 2020 13:22
@IvanGoncharov
Copy link
Member

Is it desired, or should I follow some other approach towards moving skipping functionality to parser like creating lexer.lookahead in the parser itself and renaming lexer.lookahead function to lexer._lookahed or something else?

@nveenjain If comments now included in AST as first-class nodes it means comments should be handled by the parser. So adding lookahead into parser is an appropriate solution for this problem.

But please keep comments inside the double-linked list.

P.S. Based on our Slack conversation please don't forget to investigate how comments are handled inside Babel, TS, etc. parsers. I'm especially interested in having some mechanism to allow comments to survive at least some AST modification.

@nveenjain
Copy link
Author

Based on our Slack conversation please don't forget to investigate how comments are handled inside Babel, TS, etc. parsers

@IvanGoncharov, For acorn, comments are not parsed, i.e. skipped.
For babel, comments are parsed, added to top-level as well as to internal nodes ( each node has leadingComments, and trailingComments depending on how comments are present ).

But please keep comments inside the double-linked list.

Sure will add the comments to the linked list.

Also, Do we need to add Support for leading and trailing comments too?

@nveenjain
Copy link
Author

But if we keep nodes in linked list, there might be some edge cases:-
Like for finding location, we rely on lastNode, consider the test case present in repo:-

      """
      Description
      """
      # Even with comments between them
      type Hello {
        world: String
      }

Here the description string literal will has endPosition=19 according to current implementation now, but if we add comments to linked list endPosition would become 53 (since we check the last token and it will include the comment token)...
What should I do in such case?

@nveenjain nveenjain force-pushed the feature/addCommentInAST branch from 3da9a6a to 0e85fbc Compare March 12, 2020 12:40
… feature/addCommentInAST

Signed-off-by: Naveen Jain <[email protected]>
@nveenjain nveenjain force-pushed the feature/addCommentInAST branch from 0e85fbc to 5aef47b Compare March 12, 2020 17:14
@nveenjain nveenjain requested a review from IvanGoncharov March 12, 2020 17:16
Base automatically changed from master to main January 27, 2021 11:10
@IvanGoncharov IvanGoncharov added this to the post-16.0.0 milestone Aug 12, 2021
@Vincz
Copy link

Vincz commented Dec 4, 2022

Ping @nveenjain, @IvanGoncharov
Hey guys! Any update about this feature? Having the comments in the AST would be awesome to parse/dump.
It seems this PR was almost finish, no?

@adammiller
Copy link

@nveenjain @IvanGoncharov I'd also love this feature - are you open to someone continuing this work?

I'm working on some codemod tooling to handle schema deprecations in a fleet of UI apps and a lot of the apps leverage comments in their .graphql files. This would really help with helping preserve the original document structure after transforming the AST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintain comments when parsing schema
4 participants