-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(compiler): rework diagnostics to support multiple files #414
Conversation
ooooohh that is already looking real good @goto-bus-stop ! |
6ee751f
to
7f6061c
Compare
This can be reviewed but it still has some blockers, will maintain checklist in OP |
is there a way to get ariadne to expand its spans? There are a couple of places where the fields get swallowed when diagnostics are expanded, and the error becomes a lot more confusing. Some examples:duplicate names with mietteduplicate names with ariadneexpected ident with mietteexpected ident with ariadneunexpected field with mietteunexpected field with ariadne |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some error standardisation here
Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
so at this stage it works and everything, but we have less context in error messages. |
@goto-bus-stop ok, let's merge this in, and in our continued validation process work on adding more context to ariadne |
miette
doesn't support referencing multiple files in one error, so we sadly can't use it directly. Most errors in executable documents will reference another document.Without miette's derives, we need some other way to represent shared diagnostic properties like labels and help text. For now this PR uses:
DiagnosticData
is an enum with the different diagnostic types and specific information about them.With that structure we need to format labels wherever we create an error.
I converted a part of the validation so far and explored using
ariadne
for displaying errors. I'll continue to work onariadne
output first in case the diagnostic data structure needs to change. Then some API polish, maybe we can reduce the boilerplatyness of this implementation a bit.Todos:
ariadne
release (using a fork)ApolloDiagnostic
Debug impl can censor them consistently or we just makeFileId
display as a constant value … or we change the test output to use source strings wherever aDiagnosticLocation
occurs … or something elseReset FileId between directory tests #437