Improve error messages for invalid arguments #128
Conversation
graphql/src/graphql_schema.ml
Outdated
|
|
||
| let rec string_of_const_value: Graphql_parser.const_value -> string = function | ||
| | `Null -> "null" | ||
| | `Int i -> string_of_int(i) |
There was a problem hiding this comment.
I'd consider it more idiomatic to omit parenthesis when not required, e.g. string_of_int i here. Similarly elsewhere.
graphql/src/graphql_schema.ml
Outdated
| let obj ?doc name ~fields ~coerce = | ||
| Object { name; doc; fields; coerce } | ||
|
|
||
| let rec string_of_const_value: Graphql_parser.const_value -> string = function |
graphql/src/graphql_schema.ml
Outdated
| | `Bool b -> string_of_bool(b) | ||
| | `Enum e -> e | ||
| | `List l -> Printf.sprintf "[%s]" (String.concat ", " (List.map (fun i -> (string_of_const_value i)) l)) | ||
| | `Assoc a -> Printf.sprintf "{%s}" (String.concat ", " (List.map (fun (k, v) -> Printf.sprintf "%s: %s" k (string_of_const_value v)) a)) |
There was a problem hiding this comment.
Possibly extract (List.map ...) to a let-expression (similarly above).
graphql/src/graphql_schema.ml
Outdated
| | Object(a) -> a.name | ||
| | Enum(a) -> a.name | ||
| | List(a) -> (Printf.sprintf "[%s]" (string_of_arg_typ a)) | ||
| | NonNullable(a) -> (Printf.sprintf "%s!" (string_of_arg_typ a)) |
There was a problem hiding this comment.
I would omit parenthesis in the match cases (e.g. Scalar a) and around Printf.sprintf ....
graphql/src/graphql_schema.ml
Outdated
| field_name | ||
| (match value with | ||
| | Some(v) -> Printf.sprintf "found %s" (string_of_const_value v) | ||
| | None -> "but not provided") |
There was a problem hiding this comment.
I would extract this to a let-expression.
graphql/src/graphql_schema.ml
Outdated
| with StringMap.Missing_key key -> Error (Format.sprintf "Missing variable `%s`" key) | ||
|
|
||
| and eval_arg : type a. variable_map -> a arg_typ -> Graphql_parser.const_value option -> (a, string) result = fun variable_map typ value -> | ||
| and eval_arg : type a. variable_map -> string -> string -> a arg_typ -> Graphql_parser.const_value option -> (a, string) result = fun variable_map field_name arg_name typ value -> |
There was a problem hiding this comment.
I'd consider adding labels to avoid mixing up the arguments as they have the same type (i.e. is field name first or last on invocation).
graphql/src/graphql_schema.ml
Outdated
| | Ok coerced -> Ok (Some coerced) | ||
| | Error _ -> Error (eval_arg_error field_name arg_name typ (Some value)) | ||
| in | ||
| result |
There was a problem hiding this comment.
Alternatively:
begin match s.coerce value with
...
end
|
Thanks @andreas! Updated per your comments. Any chance we could use ocamlformat on this project? https://github.com/ocaml-ppx/ocamlformat |
2 participants
I had a really hard time discovering the source of an error for a script that was making GraphQL queries to OneGraph. The error was "Invalid string", but I had multiple string arguments.
With this change, the error I get back is
Argument `pos` of type `String` expected on field `updateCard`, found 1.With that error, there is enough context for me to easily pinpoint the problem and fix it.
If you'd like an easy way to test, this change is live on OneGraph: