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

Removed anyhow #10003

Merged
merged 7 commits into from
Oct 6, 2023
Merged

Removed anyhow #10003

merged 7 commits into from
Oct 6, 2023

Conversation

bushrat011899
Copy link
Contributor

Objective

Solution

  • Added Explicit Error Typing for AssetLoader and AssetSaver, which were the last instances of anyhow in use across Bevy.

Changelog

  • Added an associated type Error to AssetLoader and AssetSaver for use with the load and save methods respectively.
  • Changed ErasedAssetLoader and ErasedAssetSaver load and save methods to use Box<dyn Error + Send + Sync + 'static> to allow for arbitrary Error types from the non-erased trait variants. Note the strict requirements match the pre-existing requirements around anyhow::Error.

Migration Guide

  • anyhow is no longer exported by bevy_asset; Add it to your own project (if required).
  • AssetLoader and AssetSaver have an associated type Error; Define an appropriate error type (e.g., using thiserror), or use a pre-made error type (e.g., anyhow::Error). Note that using anyhow::Error is a drop-in replacement.
  • AssetLoaderError has been removed; Define a new error type, or use an alternative (e.g., anyhow::Error)
  • All the first-party AssetLoader's and AssetSaver's now return relevant (and narrow) error types instead of a single ambiguous type; Match over the specific error type, or encapsulate (Box<dyn>, thiserror, anyhow, etc.)

Notes

A simpler PR to resolve this issue would simply define a Bevy Error type defined as Box<dyn std::error::Error + Send + Sync + 'static>, but I think this type of error handling should be discouraged when possible. Since only 2 traits required the use of anyhow, it isn't a substantive body of work to solidify these error types, and remove anyhow entirely. End users are still encouraged to use anyhow if that is their preferred error handling style. Arguably, adding the Error associated type gives more freedom to end-users to decide whether they want more or less explicit error handling (anyhow vs thiserror).

As an aside, I didn't perform any testing on Android or WASM. CI passed locally, but there may be mistakes for those platforms I missed.

crates/bevy_asset/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_asset/src/lib.rs Show resolved Hide resolved
crates/bevy_asset/src/lib.rs Outdated Show resolved Hide resolved
@bushrat011899 bushrat011899 marked this pull request as ready for review October 3, 2023 09:16
@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change labels Oct 5, 2023
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.

This is much nicer. Thank you!

@alice-i-cecile alice-i-cecile 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 5, 2023
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.

Thanks for diving into this. This has been bothering me since I started using Bevy. It's notably more verbose, but also much more explicit. Much more Rust-like this way. Glad to see it's finally being addressed.

@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Oct 6, 2023
@james7132 james7132 added this pull request to the merge queue Oct 6, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 6, 2023
@bushrat011899
Copy link
Contributor Author

Looks like I missed one docstring in the bevy_gltf package (which previously didn't require documentation). I've merged main back into this PR and resolved that issue.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 6, 2023
Merged via the queue into bevyengine:main with commit dd46fd3 Oct 6, 2023
24 of 25 checks passed
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

- Fixes bevyengine#8140

## Solution

- Added Explicit Error Typing for `AssetLoader` and `AssetSaver`, which
were the last instances of `anyhow` in use across Bevy.

---

## Changelog

- Added an associated type `Error` to `AssetLoader` and `AssetSaver` for
use with the `load` and `save` methods respectively.
- Changed `ErasedAssetLoader` and `ErasedAssetSaver` `load` and `save`
methods to use `Box<dyn Error + Send + Sync + 'static>` to allow for
arbitrary `Error` types from the non-erased trait variants. Note the
strict requirements match the pre-existing requirements around
`anyhow::Error`.

## Migration Guide

- `anyhow` is no longer exported by `bevy_asset`; Add it to your own
project (if required).
- `AssetLoader` and `AssetSaver` have an associated type `Error`; Define
an appropriate error type (e.g., using `thiserror`), or use a pre-made
error type (e.g., `anyhow::Error`). Note that using `anyhow::Error` is a
drop-in replacement.
- `AssetLoaderError` has been removed; Define a new error type, or use
an alternative (e.g., `anyhow::Error`)
- All the first-party `AssetLoader`'s and `AssetSaver`'s now return
relevant (and narrow) error types instead of a single ambiguous type;
Match over the specific error type, or encapsulate (`Box<dyn>`,
`thiserror`, `anyhow`, etc.)

## Notes

A simpler PR to resolve this issue would simply define a Bevy `Error`
type defined as `Box<dyn std::error::Error + Send + Sync + 'static>`,
but I think this type of error handling should be discouraged when
possible. Since only 2 traits required the use of `anyhow`, it isn't a
substantive body of work to solidify these error types, and remove
`anyhow` entirely. End users are still encouraged to use `anyhow` if
that is their preferred error handling style. Arguably, adding the
`Error` associated type gives more freedom to end-users to decide
whether they want more or less explicit error handling (`anyhow` vs
`thiserror`).

As an aside, I didn't perform any testing on Android or WASM. CI passed
locally, but there may be mistakes for those platforms I missed.
@cart
Copy link
Member

cart commented Oct 13, 2023

These AssetLoadError::CannotLoadDependency errors don't make sense to me:

  1. We're dropping the actual loader error contents, making the error message effectively useless.
  2. Not all asset loads are dependencies, but we're returning that error for every asset load failure.

@cart
Copy link
Member

cart commented Oct 13, 2023

We're also no longer including meta deserialization failures / throwing them in the generic CannotLoadDependency bucket.

@cart
Copy link
Member

cart commented Oct 13, 2023

I'm just going to fix this in Multiple Asset Sources as it touches this code.

@cart cart mentioned this pull request Oct 13, 2023
@Runi-c
Copy link

Runi-c commented Nov 2, 2023

AssetLoader and AssetSaver have an associated type Error; Define an appropriate error type (e.g., using thiserror), or use a pre-made error type (e.g., anyhow::Error). Note that using anyhow::Error is a drop-in replacement.

Not sure if a comment is the right place to put this, but this section of this PR's migration guide is incorrect. anyhow::Error does not implement Error, which is required by these associated types - so it does not work as a drop-in replacement.

ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- Fixes bevyengine#8140

## Solution

- Added Explicit Error Typing for `AssetLoader` and `AssetSaver`, which
were the last instances of `anyhow` in use across Bevy.

---

## Changelog

- Added an associated type `Error` to `AssetLoader` and `AssetSaver` for
use with the `load` and `save` methods respectively.
- Changed `ErasedAssetLoader` and `ErasedAssetSaver` `load` and `save`
methods to use `Box<dyn Error + Send + Sync + 'static>` to allow for
arbitrary `Error` types from the non-erased trait variants. Note the
strict requirements match the pre-existing requirements around
`anyhow::Error`.

## Migration Guide

- `anyhow` is no longer exported by `bevy_asset`; Add it to your own
project (if required).
- `AssetLoader` and `AssetSaver` have an associated type `Error`; Define
an appropriate error type (e.g., using `thiserror`), or use a pre-made
error type (e.g., `anyhow::Error`). Note that using `anyhow::Error` is a
drop-in replacement.
- `AssetLoaderError` has been removed; Define a new error type, or use
an alternative (e.g., `anyhow::Error`)
- All the first-party `AssetLoader`'s and `AssetSaver`'s now return
relevant (and narrow) error types instead of a single ambiguous type;
Match over the specific error type, or encapsulate (`Box<dyn>`,
`thiserror`, `anyhow`, etc.)

## Notes

A simpler PR to resolve this issue would simply define a Bevy `Error`
type defined as `Box<dyn std::error::Error + Send + Sync + 'static>`,
but I think this type of error handling should be discouraged when
possible. Since only 2 traits required the use of `anyhow`, it isn't a
substantive body of work to solidify these error types, and remove
`anyhow` entirely. End users are still encouraged to use `anyhow` if
that is their preferred error handling style. Arguably, adding the
`Error` associated type gives more freedom to end-users to decide
whether they want more or less explicit error handling (`anyhow` vs
`thiserror`).

As an aside, I didn't perform any testing on Android or WASM. CI passed
locally, but there may be mistakes for those platforms I missed.
rlidwka added a commit to rlidwka/bevy that referenced this pull request Nov 8, 2023
this fixes bevyengine#10350

Previous change made in bevyengine#10003
resulted in inability for users to have ?Sized errors, which caused
usability issues with crates like anyhow.
rlidwka added a commit to rlidwka/bevy that referenced this pull request Nov 8, 2023
this fixes bevyengine#10350

Previous change made in bevyengine#10003
resulted in inability for users to have ?Sized errors, which caused
usability issues with crates like anyhow.
@tim-blackbird tim-blackbird mentioned this pull request Nov 15, 2023
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Fixes bevyengine#8140

## Solution

- Added Explicit Error Typing for `AssetLoader` and `AssetSaver`, which
were the last instances of `anyhow` in use across Bevy.

---

## Changelog

- Added an associated type `Error` to `AssetLoader` and `AssetSaver` for
use with the `load` and `save` methods respectively.
- Changed `ErasedAssetLoader` and `ErasedAssetSaver` `load` and `save`
methods to use `Box<dyn Error + Send + Sync + 'static>` to allow for
arbitrary `Error` types from the non-erased trait variants. Note the
strict requirements match the pre-existing requirements around
`anyhow::Error`.

## Migration Guide

- `anyhow` is no longer exported by `bevy_asset`; Add it to your own
project (if required).
- `AssetLoader` and `AssetSaver` have an associated type `Error`; Define
an appropriate error type (e.g., using `thiserror`), or use a pre-made
error type (e.g., `anyhow::Error`). Note that using `anyhow::Error` is a
drop-in replacement.
- `AssetLoaderError` has been removed; Define a new error type, or use
an alternative (e.g., `anyhow::Error`)
- All the first-party `AssetLoader`'s and `AssetSaver`'s now return
relevant (and narrow) error types instead of a single ambiguous type;
Match over the specific error type, or encapsulate (`Box<dyn>`,
`thiserror`, `anyhow`, etc.)

## Notes

A simpler PR to resolve this issue would simply define a Bevy `Error`
type defined as `Box<dyn std::error::Error + Send + Sync + 'static>`,
but I think this type of error handling should be discouraged when
possible. Since only 2 traits required the use of `anyhow`, it isn't a
substantive body of work to solidify these error types, and remove
`anyhow` entirely. End users are still encouraged to use `anyhow` if
that is their preferred error handling style. Arguably, adding the
`Error` associated type gives more freedom to end-users to decide
whether they want more or less explicit error handling (`anyhow` vs
`thiserror`).

As an aside, I didn't perform any testing on Android or WASM. CI passed
locally, but there may be mistakes for those platforms I missed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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.

Reduce/remove the use of anyhow in the engine
7 participants