Closed performantdata closed 2 years ago
But the presence of this class throughout the Sangria code has been an annoyance.
Can you explain that a bit more?
Code like this isn't especially clear in the purpose of scheme
:
def parse(input: String, config: ParserConfig = ParserConfig.default)(implicit
scheme: DeliveryScheme[ast.Document]): scheme.Result =
parse(ParserInput(input), config)(scheme)
when all it really means is
def parse(input: String, config: ParserConfig = ParserConfig.default): Try[ast.Document] =
parse(ParserInput(input), config)(scheme)
It also makes code like this more opaque than need be:
case Success(res) if res.values.nonEmpty => scheme.success(res.values.head)
case Success(res) =>
scheme.failure(
new IllegalArgumentException("Input document does not contain any value definitions."))
case Failure(e: ParseError) => scheme.failure(SyntaxError(parser, input, e))
case Failure(e) => scheme.failure(e)
@yanns I posted a PR so that you can see what it looks like.
@performantdata thanks a lot for the PR that helps appreciating the impact of the change.
I'm mixed with this proposal.
On one hand, it will break quite a lot of existing APIs. It's not a major issue. We can continue the modules split with it.
On the other hand, it's a complexity that brings little value. I find unnecessary. Having a default Try
makes the code clearer.
I'm also thinking of a migration path:
DeliveryScheme.Either
and DeliveryScheme.Throw
in sangria 2.x to indicate to users to use the default one (using Try
)sangria.parser.DeliveryScheme
to indicate that it should not be imported anymore.WDYT?
we could deprecate
Yep, I assumed there'd be a PR for that.
WDYT?
Obviously, I'm in favor of the change that I proposed. Because of the "complexity that brings little value". And I doubt that anyone's actually using it, so we still have source code compatibility. And since we're doing this split in order to make the GraphQL parser available to other projects, I don't want to bake this complexity into those projects.
OK let's go for it.
OK let's go for it.
I think you have everything you need already. I'm going to base the next PRs, for #466, on these.
I'm raising this issue now, since we're talking about moving the
parser
package into its own library, andDeliveryScheme
is part of that package. But the presence of this class throughout the Sangria code has been an annoyance.Apparently Oleg got the idea from the Parboiled2 project. It was one of his earliest commits, and I suspect that it was a premature design choice. The original idea in Parboiled2 seems to be of dubious value itself.
So my suggestion is that, in the name of simplifying the code, that we return a
Try
where needed. This can easily be turned into anEither
or a thrown exception.