Variable definitions validation

[theobat]

This PR is a required step towards the resolution of #145. It improves the validation of type declaration in variable definitions. For instance it validates the fact that when a document contains $myVariable: MyVar, then the MyVar type actually exists in the schema and it's an InputType.

Remaining tasks:

• test cases for the new ValidationErrors
• fix current broken EndToEnd tests

PS: this is my first "real" attempt to write haskell so please, any insight would be greatly appreciated.

Edit: I added DefinesTypes instances on arguments because failing tests were simply not finding their argument types in the schema (schema in the sense of what is returned by makeSchema)

[jml]

Thanks! This looks very much along the lines that we spoke about, and is great work for a first serious Haskell PR.

As requested, I've tried to provide as much feedback as I can. The things marked "Nit" are ultimately unimportant things that I cannot stop myself from caring about.

[jml]

Please add a doc comment. Ideally it should say what it's used for, or why you might want to use it.

[jml]

Please put single quotes around TypeDefinition etc. This makes Haddock render them as links.

[jml]

I don't think the inner parentheses are necessary.

[jml]

Please remove all commented out code.

[jml]

Couple of things here.

First, I think this code could be written the same way as:

do
  t <- validateTypeAssertion schema var varType
  VariableDefinition var t <$> traverse validateDefaultValue value

since that's the way the Validation monad behaves.

But, you probably don't want to use the monadic behaviour. Why? Because writing this as "first validateTypeAssertion, then validateDefaultValue" implicitly communicates that validateDefaultValue depends on validateTypeAssertion. But of course it doesn't.

From a user experience point of view, this means if there's a problem with the type assertion and a problem with the default value, they will only get the error from the type assertion. This is not ideal.

Instead, you want to use the applicative behaviour. e.g.

  VariableDefinition var <$> validateTypeAssertion schema var varType <*> traverse validateDefaultValue value

This clearly shows that the two are independent, and the Validation applicative instance will make sure both errors are included (if there happen to be errors from both).

Does this make sense?

[theobat]

Yes. Not sure I thoroughly grasp the entire concepts of Applicative and Monad, but as far as this example is concerned I think I understand.

[jml]

Nit: capital first letter, capitalize "AST" and end with a full stop.

[jml]

schemaTypeName, astTypeName

[jml]

This looks wrong. The annotated type of "list of int" should be "list of int", not "int".

[jml]

Why does this take a Variable?

[theobat]

It takes a variable name because it enables us to give the variable name in the error message (if the given type is not a builtin type, which is the error message if we give a random type name for instance). I do believe this is valuable.

[jml]

Nit: Please don't add unnecessary whitespace.

[theobat]

@jml, Now I think I'm done here, for some reason the tests are marked as failed on CircleCI, but there isn't any error at any stage of the pipeline and the coverage increased everywhere:

#!/bin/bash -eo pipefail
STACK_YAML=stack-8.2.yaml ./scripts/hpc-ratchet

alternatives: BETTER (175 missing => 161 missing)
booleans: OK
expressions: BETTER (1494 missing => 1416 missing)
local_decls: BETTER (15 missing => 14 missing)
top_level_decls: BETTER (685 missing => 669 missing)
Exited with code 2

I'm still open to other changes though if things are not right yet.

[jml]

@theobat Thanks, I'll take a look now.

To fix the coverage error, you need to update the numbers in the script to match the new, better numbers: https://github.com/haskell-graphql/graphql-api/blob/master/scripts/hpc-ratchet#L37-L43

(Edit: correct link)

[jml]

This looks great, thanks! I'm super excited to see the improved test coverage.

One minor comment.

[jml]

Not sure I could do this todo. Please either do it or expand on it so that someone who's not you could do it.

[theobat]

So this is just a reference to the one above that I did not write.

-- TODO: This error message is pretty bad. We should define
-- a typeclass for client-friendly "Show" (separate from
-- actual Show which remains extremely useful for debugging)
-- and use that when including values in error messages.

Now, while I understand both the idea and how to do it (I think...), it seems like a lot of work for this PR. Since both error messages are pretty close, I thought it would be better to add the TODO reference to the other one, but now that I'm reading it on github, it seems rather unhelpful. I'm going to simply remove it and let the original TODO as it is

[theobat]

@jml There we are ! Thanks for your reviews. As a side note, it seems like every guard containing an otherwise statement is counted as "always true" by hpc which means increasing the number of otherwise might force us to decrease coverage..

57% guards (11/19), 6 always True, 2 unevaluated

Next step is a PR with the actual Aeson.Object to Value translation. I won't address it before some time though.

[jml]

As a side note, it seems like every guard containing an otherwise statement is counted as "always true" by hpc

Yeah, that's something I really would like to fix in hpc (or with some kind of tooling), rather than compromising on coverage. The Haskell testing ecosystem needs to get better.

[jml]

Thanks!