graphql-dotnet / parser

A lexer and parser for GraphQL in .NET
MIT License
216 stars 43 forks source link

Handle whitespaces somehow #360

Open sungam3r opened 1 year ago

sungam3r commented 1 year ago

@my @your instead of @my @your now. It could be fixes as well checking parent node but for directives parent is always not null - it is Directives container, so actually grand parent is needed. Moreover, even after such check some tests failed (see Printer_Should_Print_Pretty_If_Directives_Skipped). The core problem is that some arbitrary nodes can be skipped so it's not obvious how to handle whitespaces at all. General approach may be to require each node to print their leading whitespace if it should do that. Downside - it will complicate all printing code. I do not want to do that just to handle very rare cases.

_Originally posted by @sungam3r in https://github.com/graphql-dotnet/parser/pull/359#discussion_r1360220872_

Shane32 commented 1 year ago

Have you thought about making an protected/private method which deduplicates whitespace by examining the leading/trailing character to remember if it's whitespace (space or CR/LF)? Then just strip leading space character if it's a duplicate (unless indenting)? No allocation when stripping whitespace because we are using ROMs.

This change could likely be made almost entirely within PrintContextExtensions.WriteAsync with an extra overload or method to support indenting. Minor changes would need to be made to WriteEncodedStringAsync and WriteMultilineBlockString also so that no whitespace is stripped from within a string.

Shane32 commented 1 year ago

Taking this a step further, if we abstract all of the formatting away from the SDLPrinter into the print context, we could add an option to print 'minimized', handled entirely by the print context. It would just strip or ignore all whitespace except where needed, detected solely by the last character printed compared with the next character to print. So "d" and "(" do not require a space between them in "field (arg: 2)", and nor does ":" and "2". So the print context might have methods such as:

sungam3r commented 1 year ago

I thought a little about all this. In general, an additional state in context and management of that state will be required.