move parser into its own module

[performantdata]

Fixes #466. Based on PR #790.

[performantdata]

@yanns Where do you think StringUtil.blockStringValue should go? Rip out a copy into the parser module? It only appears to be used there. Then deprecate in core, 2.x branch?

[yanns]

@yanns Where do you think StringUtil.blockStringValue should go? Rip out a copy into the parser module? It only appears to be used there. Then deprecate in core, 2.x branch?

Yes we can move it into the parser module.
To avoid breaking changes, we can keep StringUtil.blockStringValue calling the implementation in the parser module.

[yanns]

You can rebase on main and force-push it now.

[performantdata]

calling the implementation in the parser module

I'm making that private, so just leaving the full implementation in StringUtil. None of the other Sangria core code is using blockStringValue, so at this point it's a method that we provide for our users? That seems unlikely. I suspect that it was only ever intended for the parser, so I would deprecate it.

[performantdata]

leaving the full implementation in StringUtil. None of the other Sangria core code is using blockStringValue, so at this point it's a method that we provide for our users? That seems unlikely. I suspect that it was only ever intended for the parser, so I would deprecate it.

Should I delete this method in this PR? and deprecate it in 2.x?

[performantdata]

Should I deprecate the QueryParser members in the 2.x branch? I made them private[this] or protected, and sealed the traits. I assume not, since I assume that no one is using them directly.

[yanns]

Should I deprecate the QueryParser members in the 2.x branch? I made them private[this] or protected, and sealed the traits. I assume not, since I assume that no one is using them directly.

no I'd not do that.
What is the intended purpose of using sealed traits? Do you want to make sure that no one is using those traits outside the sangria parsed?

[yanns]

leaving the full implementation in StringUtil. None of the other Sangria core code is using blockStringValue, so at this point it's a method that we provide for our users? That seems unlikely. I suspect that it was only ever intended for the parser, so I would deprecate it.

Should I delete this method in this PR? and deprecate it in 2.x?

I think that this implementation should be moved to its own class as it's something present in the specs: http://spec.graphql.org/October2021/#sec-String-Value.Block-Strings

The existing tests should be also extracted in a separated class.

And StringUtil.blockStringValue should call this implementation.

[performantdata]

Should I deprecate the QueryParser members in the 2.x branch? I made them private[this] or protected, and sealed the traits. I assume not, since I assume that no one is using them directly.

no I'd not do that.

OK, so I won't "deprecate the QueryParser members in the 2.x branch".

What is the intended purpose of using sealed traits? Do you want to make sure that no one is using those traits outside the sangria parsed?

Right. Since I'm assuming also that no one is currently using the traits directly, I see no reason to make them part of the usable public API. That would restrict our freedom to refactor.

It also might buy some performance, depending on the assumptions that the compiler makes. I know that works for visibility: private[this] is usually inlined.

[performantdata]

I think that this implementation should be moved to its own class as it's something present in the specs

And StringUtil.blockStringValue should call this implementation.

But are you sure that anyone's using it? I don't see it being called from open source Scala code on GitHub. Deprecating the method and seeing who complains might make more sense than needlessly exposing parser internals.

(I made the changes locally, but paused on this thought before committing.)

[yanns]

I think that this implementation should be moved to its own class as it's something present in the specs

And StringUtil.blockStringValue should call this implementation.

But are you sure that anyone's using it? I don't see it being called from open source Scala code on GitHub. Deprecating the method and seeing who complains might make more sense than needlessly exposing parser internals.

(I made the changes locally, but paused on this thought before committing.)

Yes, we can deprecate StringUtil.blockStringValue.

[performantdata]

Anything else? Do we want to make sangria-parser a Java 9 module? (The purpose of doing that would be the same: ensure that visibility is restricted to a desired-public API.)

Actually, I should hold off on that. I don't know enough to make that happen quickly.

[yanns]

I've run this version with some applications, and no issues detected!
👍

[yanns]

Anything else? Do we want to make sangria-parser a Java 9 module? (The purpose of doing that would be the same: ensure that visibility is restricted to a desired-public API.)

Actually, I should hold off on that. I don't know enough to make that happen quickly.

Yes, it's something we can add later if we need.