Fix NRE when printing directive without locations

[sungam3r]

Edge case in tests for https://github.com/sungam3r/graphql-introspection-model

[codecov-commenter]

Codecov Report

Merging #300 (c6a4c8d) into master (b05f365) will increase coverage by 0.00%.
The diff coverage is 100.00%.

📣 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     #300   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          85       85           
  Lines        4540     4547    +7     
  Branches      459      461    +2     
=======================================
+ Hits         4532     4539    +7     
  Misses          6        6           
  Partials        2        2           

Impacted Files	Coverage Δ
src/GraphQLParser/Visitors/SDLPrinter.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.

[Shane32]

The real problem is that we have = null!; as the property initializer, which we should not if we are exposing the class publicly.

[sungam3r]

I can argue. = null! is done by following the GraphQL grammar - the AST node for locations is not optional. For optional nodes we do have ? in their property type.

[sungam3r]

I mean any property in AST node may be uninialized now if constructed manually. Parser throws exception if locations not set into parsed text.

[Shane32]

Right but what I mean is that we should have a constructor which requires a value and remove the default constructor, in order for the NRT attributes to be accurate always. Then “= null!” is not required. Or, make the default constructors internal.

Certainly the NRT attributes are correct as-is to match the AST parsing.

[sungam3r]

It will force us to switch the order of nodes creation from "parent, then child" to "child, then parent" and
may give unforseen consequences.

[Shane32]

Internally we can still do it the other way. But the public constructor would have proper NRT attributes. We can also allow for users to pass in null! if they like to the constructor, but they are then knowingly breaking NRT semantics.

[sungam3r]

Understood.