Skip to content
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

Derive Error for more error types #10240

Merged
merged 4 commits into from
Oct 28, 2023

Conversation

killercup
Copy link
Contributor

Objective

Align all error-like types to implement Error.

Fixes #10176

Solution

  • Derive Error on more types
  • Refactor instances of manual implementations that could be derived

This adds thiserror as a dependency to bevy_transform, which might increase compilation time -- but I don't know of any situation where you might only use that but not any other crate that pulls in bevy_utils.

The contributors example has a LoadContributorsError type, but as it's an example I have not updated it. Doing that would mean either having a use bevy_internal::utils::thiserror::Error; in an example file, or adding thiserror as a dev-dependency to the main bevy crate.


Changelog

  • All …Error types now implement the Error trait

@killercup
Copy link
Contributor Author

killercup commented Oct 23, 2023

This covers all types found by (struct|enum).*Error. If you know of more types or a better pattern, let me know.

@killercup killercup force-pushed the feature/impl-error branch 2 times, most recently from 15ebdc4 to 544a6c9 Compare October 23, 2023 14:30
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This adds thiserror as a dependency to bevy_transform, which might increase compilation time -- but I don't know of any situation where you might only use that but not any other crate that pulls in bevy_utils.

I think it already is a transitive dependency via bevy_ecs, so this might not be a significant change.

The contributors example has a LoadContributorsError type, but as it's an example I have not updated it. Doing that would mean either having a use bevy_internal::utils::thiserror::Error; in an example file, or adding thiserror as a dev-dependency to the main bevy crate.

Note that the bevy_utils crate should still be accessible via bevy::utils, so bevy::utils::thiserror

@killercup
Copy link
Contributor Author

killercup commented Oct 24, 2023

I don't think updating the example blocks this PR.


Note that the bevy_utils crate should still be accessible via bevy::utils, so bevy::utils::thiserror

Are we committed enough to this dependency to promote it in our examples?

But also with use bevy::utils::thiserror::Error;, I get this:

error[E0433]: failed to resolve: use of undeclared crate or module `thiserror`
   --> examples/games/contributors.rs:315:17
    |
315 | #[derive(Debug, Error)]
    |                 ^^^^^ use of undeclared crate or module `thiserror`
    |
    = note: this error originates in the derive macro `Error` (in Nightly builds, run with -Z macro-backtrace for more info)

However (and unintuitively), this works:

use bevy::utils::thiserror;

#[derive(Debug, thiserror::Error)]
enum LoadContributorsError {
    #[error("An IO error occurred while reading the git log.")]
    Io(#[from] io::Error),
    #[error("The CARGO_MANIFEST_DIR environment variable was not set.")]
    Var(#[from] VarError),
    #[error("The git process did not return a stdout handle.")]
    Stdout,
}

If you think this is a good example, I can push a commit with this change.

@alice-i-cecile
Copy link
Member

Are we committed enough to this dependency to promote it in our examples?

IMO yes: it's an ecosystem standard and really helpful to teach Rust newcomers (even if it's not particularly Bevy-related).

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use labels Oct 24, 2023
@killercup
Copy link
Contributor Author

IMO yes: it's an ecosystem standard and really helpful to teach Rust newcomers (even if it's not particularly Bevy-related).

Alright! Updated the example, too.

This adds thiserror as a dependency to this crate. This might mean it
pulls in syn now.
Note that thiserror was already a dependency, so this is just cleaning
up the code and making it more concise.
This uses the reexport from `bevy::utils::thiserror`.
Please note that the module has to be imported, not just the `Error`
type, as macro relies on `thiserror` to be in scope.
@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 28, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 28, 2023
Merged via the queue into bevyengine:main with commit 0c2c52a Oct 28, 2023
26 checks passed
@killercup killercup deleted the feature/impl-error branch October 31, 2023 10:10
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

Align all error-like types to implement `Error`.

Fixes  bevyengine#10176

## Solution

- Derive `Error` on more types
- Refactor instances of manual implementations that could be derived

This adds thiserror as a dependency to bevy_transform, which might
increase compilation time -- but I don't know of any situation where you
might only use that but not any other crate that pulls in bevy_utils.

The `contributors` example has a `LoadContributorsError` type, but as
it's an example I have not updated it. Doing that would mean either
having a `use bevy_internal::utils::thiserror::Error;` in an example
file, or adding `thiserror` as a dev-dependency to the main `bevy`
crate.

---

## Changelog

- All `…Error` types now implement the `Error` trait
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Align all error-like types to implement `Error`.

Fixes  bevyengine#10176

## Solution

- Derive `Error` on more types
- Refactor instances of manual implementations that could be derived

This adds thiserror as a dependency to bevy_transform, which might
increase compilation time -- but I don't know of any situation where you
might only use that but not any other crate that pulls in bevy_utils.

The `contributors` example has a `LoadContributorsError` type, but as
it's an example I have not updated it. Doing that would mean either
having a `use bevy_internal::utils::thiserror::Error;` in an example
file, or adding `thiserror` as a dev-dependency to the main `bevy`
crate.

---

## Changelog

- All `…Error` types now implement the `Error` trait
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change C-Usability A simple 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.

Add Error derive to all Bevy error types
3 participants