remove DeliveryScheme

[performantdata]

I'm raising this issue now, since we're talking about moving the parser package into its own library, and DeliveryScheme 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 an Either or a thrown exception.

[yanns]

But the presence of this class throughout the Sangria code has been an annoyance.

Can you explain that a bit more?

[performantdata]

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)

[performantdata]

@yanns I posted a PR so that you can see what it looks like.

[yanns]

@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:

• we could deprecate DeliveryScheme.Either and DeliveryScheme.Throw in sangria 2.x to indicate to users to use the default one (using Try)
• we could also deprecate the object sangria.parser.DeliveryScheme to indicate that it should not be imported anymore.

WDYT?

[performantdata]

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.

[yanns]

OK let's go for it.

[performantdata]

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.