-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add error codes #457
Add error codes #457
Conversation
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.
This is looking good! I really like the idea of having the errors be in markdown so that we can have some error reference pages in the docs (perfect for SEO on running into errors!) Agree that for printing them out maybe we should try pretty printing the markdown :)
Co-authored-by: Avery Harnish <avery@apollographql.com>
Since I'm replacing the markdown urls like |
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.
looking good!
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.
This is in a really good shape. I absolutely love the rover explain
command, this will be so handy!
Left a few comments about the Code
API which may perhaps help with its usage.
…lographql/rover into jake/error-codes-coding-errors-code
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.
LGTM!
.filter_map(Result::ok) | ||
.filter(|e| !e.file_type().unwrap().is_dir()); |
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.
should we maybe error here instead?
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.
We definitely could 🤷♂️ Especially since this is at buildtime. I just left it as the simpler option in the meantime. Do you think I should change that?
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.
yeah since it's at build time i think maybe we should error here so we don't commit something that doesn't work, i imagine filtering out the errors could end up with some head scratches if something goes wrong
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.
I am going to keep this as is for now and we can get it to error in an xtask refactor.
This PR adds usable error codes, and associates them with all errors that are handled by Rover.
By usable, I mean there's a new
rover explain <CODE>
command to explain a single error codeI took inspiration for the explanations themselves being in markdown from https://github.com/rust-lang/rust/blob/master/compiler/rustc_error_codes/src/error_codes.rs
We use termimad to do some basic formatting on the markdown explanations, which also gives us some flexibility to make wrapping tables, etc in our explanations. The only odd thing is that it doesn't support markdown links, so those are just printed as markdown
There are also changes to
build.rs
that iterate over all files in thecodes
dir and prints their contents to aerrors
docs for a reference pagerover explain E001