Skip to content

[Merged by Bors] - add detailed errors#2994

Closed
mockersf wants to merge 4 commits intobevyengine:mainfrom
mockersf:error-codes
Closed

[Merged by Bors] - add detailed errors#2994
mockersf wants to merge 4 commits intobevyengine:mainfrom
mockersf:error-codes

Conversation

@mockersf
Copy link
Member

Objective

  • Improve error descriptions and help understand how to fix them
  • I noticed one today that could be expanded, it seemed like a good starting point

Solution

I decided to start the error code with B for Bevy so that they're not confused with error code from rust (which starts with E)

Longer term, there are a few more evolutions that can continue this:

  • the code samples should be compiled check, and even executed for some of them to check they have the correct error code in a panic
  • the error could be build on a page in the website like https://doc.rust-lang.org/error-index.html
  • most panic should have their own error code

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 20, 2021
@mockersf mockersf added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Oct 20, 2021
Copy link
Member

@TheRawMeatball TheRawMeatball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the "ennemy" typo that seems to be pretty common, this looks like a good change.
Two notes:

  • figuring out how to get from a code like [B0002] to the relevant page isn't obvious. We should probably also print a link to the relevant page.
  • I don't think we need "complete" snippets with a prelude import and an fn main. Feels like it just introduces unnecessary noise in this case.

@mockersf
Copy link
Member Author

Apart from the "ennemy" typo that seems to be pretty common

It has two "n" in French...

@mockersf
Copy link
Member Author

  • figuring out how to get from a code like [B0002] to the relevant page isn't obvious. We should probably also print a link to the relevant page.

How rust does it:

  • the error are on a known page, it's the tool displaying the error responsibility to correctly link the error page. This could be done once we have the Bevy editor capable of running a Bevy game
  • rustc --explain e0001 display the error markdown as they are all compiled into rustc. We could have the same thing once we have a Bevy CLI

We could also print the link directly, but hopefully we would set up the proper deployment on the Bevy website first

@mockersf
Copy link
Member Author

  • I don't think we need "complete" snippets with a prelude import and an fn main. Feels like it just introduces unnecessary noise in this case.

I would like to compile check them later on, so that needs a complete snippet...

@TheRawMeatball
Copy link
Member

I would like to compile check them later on, so that needs a complete snippet...

Makes sense, but I think we should introduce the boilerplate after we have that hooked up, and, ideally use something like what rustdoc does and hide the boilerplate. Also, maybe we should move these into the new book? I'm pretty sure the new book does support compiler checking, and it'd also be in a more accessible place.

@IceSentry
Copy link
Contributor

I don't think it needs a link, the error message is clear enough as it is.

As for compile checking it, I believe there's issues with the book on that front right now, so I wouldn't block this for that.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any tools we can use to record the current highest error code in use is?

Can we automatically verify that there's no conflicts?

@alice-i-cecile
Copy link
Member

Cool; I really like this, and the error messages themselves are good. I'm a bit worried about maintainability (see the above comment), but I'm basically on board. This reminds me a lot of https://bevy-cheatbook.github.io/pitfalls/_index.html by @inodentry: hopefully we can slowly improve the error messages and shrink that section of the Cheatbook.

@mockersf
Copy link
Member Author

Are there any tools we can use to record the current highest error code in use is?

Can we automatically verify that there's no conflicts?

It's backed by files, don't use a code if there's no file for it. The highest one is the last file added. If there's no git conflict on the file, then there's no conflict.

@NiklasEi
Copy link
Member

NiklasEi commented Oct 20, 2021

We could think about a workflow that checks that there is an error file for every error code in the code. But I think for now, while the number of codes is pretty low, it's fine without that.

By the way: Out of interest in Zola I started something to integrate the error codes in the website. What do you think about a list under /errors and the markdown files at /errors/{code}?

Edit: I now prefer one long list including the markdown for every code like rust does it. I don't think we need the code specific pages, just tags to link to certain codes.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 20, 2021
@alice-i-cecile
Copy link
Member

Actually @mockersf can we add this to the CONTRIBUTING.md page too? Knowing how and where to extend these will be helpful.

@mockersf
Copy link
Member Author

Actually @mockersf can we add this to the CONTRIBUTING.md page too? Knowing how and where to extend these will be helpful.

I took a quick look at the file, didn't see a place where I could add that info that wouldn't be too heavy...

also fix a few ennemies that where still remaining...
@alice-i-cecile
Copy link
Member

#3071 is a good candidate for future inclusion.

@cart
Copy link
Member

cart commented Nov 6, 2021

I love this! Very happy with the overall structure and the initial codes chosen.

@cart
Copy link
Member

cart commented Nov 6, 2021

bors r+

bors bot pushed a commit that referenced this pull request Nov 6, 2021
# Objective

- Improve error descriptions and help understand how to fix them
- I noticed one today that could be expanded, it seemed like a good starting point

## Solution

- Start something like https://github.com/rust-lang/rust/tree/master/compiler/rustc_error_codes/src/error_codes
- Remove sentence about Rust mutability rules which is not very helpful in the error message

I decided to start the error code with B for Bevy so that they're not confused with error code from rust (which starts with E)


Longer term, there are a few more evolutions that can continue this:
- the code samples should be compiled check, and even executed for some of them to check they have the correct error code in a panic
- the error could be build on a page in the website like https://doc.rust-lang.org/error-index.html
- most panic should have their own error code
@bors bors bot changed the title add detailed errors [Merged by Bors] - add detailed errors Nov 6, 2021
@bors bors bot closed this Nov 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants