TheEmeraldBee / widgetui

A bevy systems like widget system for ratatui and crossterm
MIT License
38 stars 5 forks source link

Switch all uses of `Result<T, Box<dyn Error>>` to `anyhow::Result<T>`. #6

Closed emilyyyylime closed 6 months ago

emilyyyylime commented 6 months ago

I tried using this crate in my app, but I found that the error types returned are significantly under-constrained, such that I couldn't use this crate within asynchronous code. (The error type isn't : Send)

It seems like anyhow is already brought in as a dependency, only to be used once as return Err(anyhow!("...").into()) (src/chunks.rs:34)—given that, I decided we might as well change all Results to use anyhow::Result, with an import at the top of each file.[^1]
One alternative would be to use std::io::Result, which would slot in correctly in almost every return type across the codebase, or to simply add the constraints necessary for anyhow::Error as Box<dyn Error + Send + Sync + 'static>.

Unfortunately, it seems all of these changes have the ability to break existing code, and should thus require a semver bump. However; I'm not so sure any real usage of the crate would break---as normal error propagation is done using the ? operator, which can convert error types to the end users' need.

[^1]: Let me know if you prefer a different style, like defining the alias once at the crate root, or using the full path anyhow::Result<T> every time—or feel free to edit the PR yourself ^^.

emilyyyylime commented 6 months ago

Also not sure if the Quickstart section of the README should be changed.

emilyyyylime commented 6 months ago

ugh, forgot to switch to the branch ._.

TheEmeraldBee commented 6 months ago

I'm currently writing to use thiserror to improve underlying error handling. This will use an enum that will represent all error types in the crate, with a anyhow::Error that will be used if none of the errors match. In most situations, this shouldn't change how code works, while allowing for future changes to enable improved error handling in the underlying runtime.

TheEmeraldBee commented 6 months ago

These changes have just been pushed alongside the 0.6.0 update. Thank you for pointing this out!

emilyyyylime commented 6 months ago

Actually, for me the biggest issue was from App::new—which can just be changed to return io::Result, along with many other functions. I honestly don't think you even need anyhow or Box<dyn Error> now that you've introduced thiserror, you should definitely give that a shot

TheEmeraldBee commented 6 months ago

Thank you for pointing this out, I totally missed these situations. The reason I would still like to have Anyhow be used is so that errors that should crash the program can still use a ? operator to return the error instead of panicking using unwrap. Either way, this has been fixed, I am currently pushing these changes.