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

AST printer #20

Closed
wants to merge 2 commits into from
Closed

AST printer #20

wants to merge 2 commits into from

Conversation

mkmarek
Copy link
Member

@mkmarek mkmarek commented Sep 18, 2018

No description provided.

@mkmarek mkmarek changed the title WIP: AST printer AST printer Sep 21, 2018
@sungam3r sungam3r added the enhancement New feature or request label Dec 4, 2019
@Shane32
Copy link
Member

Shane32 commented Jan 15, 2021

I think this should code should write values to a TextWriter - no concatenation. Ideally it should support both synchronous and asynchronous writing in case the target is a socket or filestream. Conversion to a string can be handled by a StringWriter (which is backed by a StringBuilder).

@sungam3r
Copy link
Member

@Shane32 Do you want to work on this?

@Shane32
Copy link
Member

Shane32 commented Jan 15, 2021

I think we should remove the AST model tree from GraphQL.NET, and rewrite the library to simply use the classes provided within this project. Optionally, we could move the AST classes to a separate project that provides the models only (e.g. GraphQL.Language.Abstractions), and both projects could rely on that abstraction library of models. It would save a lot of cpu time/memory that currently just is wasted copying information. Then I think we may be able to remove the AST printer from there, and add it here, which seems to be a more logical place for it (being both I/O for query to string translation).

If you would agree that it is our goal, then I could be talked into working on it. It wouldn't be hard. But I don't have any need for two AST printers, let alone one.

@Shane32
Copy link
Member

Shane32 commented Jan 15, 2021

Actually the SchemaPrinter might use a Schema, not AST. So these printers may be different. I'm not sure as I've never used a SchemaPrinter or AST printer. Perhaps, however, they could share code. Not sure without digging deeper.

@sungam3r
Copy link
Member

Let's postpone this for the next major version (not "current next").

@sungam3r sungam3r mentioned this pull request Nov 13, 2021
@sungam3r sungam3r added this to the 8.0 milestone Nov 13, 2021
@sungam3r sungam3r added the duplicate This issue or pull request already exists label Nov 13, 2021
@sungam3r
Copy link
Member

@mkmarek @Shane32 I close this in favor of #149.

@sungam3r sungam3r closed this Nov 13, 2021
@sungam3r sungam3r deleted the feature/ast-printer branch January 2, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants