Skip to content

Keep comments when printing a parsed query #1029

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

Closed
wants to merge 1 commit into from

Conversation

astorije
Copy link
Contributor

@astorije astorije commented Sep 9, 2017

WIP WIP WIP

This follows attempt and discussion started at graphql/graphiql#578.

Opening this early to validate: am I going into the wall here?

Because the printer takes a parsed AST as an input, what this is doing for now is trying to keep comments in said parsed AST.
But is it something actually reasonable? Should comments be kept in the AST? If not, I'm not sure how to achieve this without changing the printer so that it takes the result of lexer as an input instead of the result of parse.

I was recommended in the link above to take example from prettier which already supports printing comments (as well as other goodies such breaking arguments into multiple lines when a line gets too long), but being entirely new to both codebases, it's rather difficult for me to understand, at least for now.

I'm more than happy to spend more time on this as I do want to achieve this, but any help would be welcome here :)

Copy link
Contributor

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait on the review. There are a few critical issues to address to move this forward.

First: There's no need to add new changes to the parser or lexer since comments are already included during parse. Comment nodes may appear anywhere, so they cannot be part of the structural AST. Instead they are part of the lexing stream. The lexer returns a linked list of nodes which include comments. This fact is already exploited by the (now deprecated) use of comments as descriptions (https://github.com/graphql/graphql-js/blob/master/src/utilities/buildASTSchema.js#L483-L502)

Next, the existing printer should either not be changed in favor of a new printWithComments or should take options to print comments, the default being disabled - this will preserve existing behavior. I'm not sure which of the two will be best, that may depend on implementation. If supporting printing comments is a minor addition then a option may be better, but if supporting printing comments is a total refactor of the printer then perhaps a new function is needed

@PixnBits
Copy link

PixnBits commented Apr 5, 2019

lexer.advance()...skips all comments (the same double-linked-list test above checks that).

I saw the same problem, but then found that descriptions have been changed from comments to string literals. There is a commentDescriptions option (part of the BuildSchemaOptions TS type) that explains this a bit more (including deprecation plans).

Changing the descriptions in my schema from # description to "description" retained the descriptions through print(parse()) using v14.2.1 of graphql-js.

This PR may not be needed anymore? (see below)

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Apr 5, 2019

@PixnBits It became a less urgent issue after descriptions were moved to string literals.
However it still important if you want to prettify query/SDL without loosing comments like prettier does, see #1799

@astorije
Copy link
Contributor Author

I'm going to close this to reduce clutter on this repo, as I'm not going to be able to work on it anytime soon. Anyone who feels like it can help taking over :)
Thanks for the support, everyone!

@astorije astorije closed this Jul 21, 2019
@astorije astorije deleted the astorije/print-comments branch July 21, 2019 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants