apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
566 stars 45 forks source link

Add ariadne wrapper `ToDiagnostic` / `CliReport` #747

Closed goto-bus-stop closed 9 months ago

goto-bus-stop commented 10 months ago

This is an attempt to provide a reusable API on top of ariadne that downstream users can use to get similar formatting to apollo-rs. There is especially a need for this as ariadne uses character indices while apollo-rs uses byte indices, so we need to convert the numbers in between.

This adds public API:

This API requires that users store the error location inside their own error type.

Inconveniences:

The things to do here:

SimonSapin commented 9 months ago

https://github.com/apollographql/apollo-rs/pull/764 adds DiagnosticList::merge which internally merges two SourceMap. https://github.com/apollographql/apollo-rs/pull/764#discussion_r1410910183 points out that may be useful separately, but SourceMap is a type alias for foreign types so we can have an inherent method SourceMap::merge.

goto-bus-stop commented 9 months ago

This now always uses colors and strips them out at the end, because ariadne adds/strips colors as soon as you add a label, but here we would normally add the labels first and then decide whether they should be colored when it's time to print.

I added write and fmt inherent methods that consume self so we only need one type. You have to pass in a SourceMap in order to render, and you have to use the fmt method inside your own fmt::{Display,Debug} implementation, but you can't pass in a SourceMap into your own fmt::{Display,Debug} implementation except for having it available on the object. Soo yes this way would require exposing Diagnostic (and DiagnosticList probably) for people's custom errors, probably with a trait, which feels heavyweight, or expecting users to stick a SourceMap in their errors wherever they are produced. Will experiment with it a bit in fed-next :)

goto-bus-stop commented 9 months ago

Going to explore using the Diagnostic{sources, inner} type that we use with a generic inner error. You would have to wrap your error type with Diagnostic<> to print it with pretty formatting. One motivation for that is that inner errors would commonly derive thiserror, so they already have a fmt::Display implementation, and can't then also have pretty printing. (Unless users avoid thiserror and instead use DiagnosticReport, which would be less nice) The inner type would have to implement a trait method to produce a diagnostic report which Diagnostic::fmt would use.

SimonSapin commented 9 months ago

I made some tweaks, added a changelog, and merged to stop myself from making more tweaks :sweat_smile: