graphql-dotnet / parser

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

Refactor SDLPrinter for better indentation #304

Closed sungam3r closed 1 year ago

sungam3r commented 1 year ago

fixes #287 waits #303 to be merged into

The core idea of this PR - manage indentation more centrally. Side note - I tried to get rid of trailing NewLine when printing AST.

sungam3r commented 1 year ago

I'm sure there are more issues to fix but for now I finished. I simplified working with indents, especially for comments and descriptions and added more tests to verify edge cases.

codecov-commenter commented 1 year ago

Codecov Report

Merging #304 (bca08ee) into master (6587b6f) will decrease coverage by 0.05%. The diff coverage is 99.23%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
- Coverage   99.82%   99.77%   -0.05%     
==========================================
  Files          85       85              
  Lines        4556     4467      -89     
  Branches      462      448      -14     
==========================================
- Hits         4548     4457      -91     
- Misses          6        8       +2     
  Partials        2        2              
Impacted Files Coverage Δ
src/GraphQLParser/Visitors/SDLPrinter.cs 99.73% <99.15%> (-0.27%) :arrow_down:
...rser/AST/Definitions/GraphQLEnumValueDefinition.cs 100.00% <100.00%> (ø)
...QLParser/AST/Definitions/GraphQLFieldDefinition.cs 100.00% <100.00%> (ø)
...ser/AST/Definitions/GraphQLInputValueDefinition.cs 100.00% <100.00%> (ø)
...hQLParser/AST/Definitions/GraphQLTypeDefinition.cs 100.00% <100.00%> (ø)
...c/GraphQLParser/Visitors/PrintContextExtensions.cs 100.00% <100.00%> (ø)
src/GraphQLParser/Visitors/StructurePrinter.cs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sungam3r commented 1 year ago

So what about version? 8.3 or 9.0? I lean to 8.3 since printer is auxiliary code and not too many people should use custom contexts for printing.

sungam3r commented 1 year ago

More argument against 9.0 - it will require to bump version of core report and server.

Shane32 commented 1 year ago

Your call. If it’s source compatible then I’m okay with 8.3.

The core repo and server specify 8.2+ probably and don’t need to bump to 9.0 if they don’t rely on any breaking API.

Shane32 commented 1 year ago

Whatever you think I guess. It really only matters to you if the features are not used by GraphQL. I’d just publish as 9.0 and leave GraphQL 7.x using parser 8.x until we bump GraphQL to 8.0. Again your call.

sungam3r commented 1 year ago

It is not source compatible in case of custom printer context implemented in user code. I added new members in public interface.

Shane32 commented 1 year ago

I’m okay with default implementation of new interface members. That reduces breaking changes to .NET Framework users. (Or older obsolete Core users)

sungam3r commented 1 year ago

I have never used that feature before, will look tomorrow.

sungam3r commented 1 year ago

Instance state is not allowed for default interfaces so I'll bump to 9.