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

Consider adding anyhow as a dependency #24

Closed
odilf opened this issue Feb 6, 2024 · 4 comments · Fixed by #25
Closed

Consider adding anyhow as a dependency #24

odilf opened this issue Feb 6, 2024 · 4 comments · Fixed by #25

Comments

@odilf
Copy link
Contributor

odilf commented Feb 6, 2024

I have been using this crate for a project and it is fantastic! However, there is one problem: it doesn't play nicely with anyhow. This is mainly because the error doesn't implement Send and Sync. While looking through the code I realized that there is an error module that does things very similarly to anyhow. In fact, it would be very easy to change all errors and results to the anyhow equivalents. That would also bring along all other benefits of anyhow such as the size of struct being only a usize, better ergonomics, etc.

I understand that this project has a "no dependency" rule. I don't know if that's a hard rule for you, but I would consider changing it. Currently it seems that the error module is just a worse version of anyhow, so why not add actual anyhow as a dependency? The only concrete reason I can think of for not using it as a dependency is because of compile time concerns, but it's usually faster because crates can be compiled in parallel and compilations shared between subdependencies. Rust has little of the disadvantages that some other languages have with deps. Re-inventing the wheel in this way is very rarely a good idea.

I understand that sometimes open source contributors get a lot of often unreasonable demands, so I want to reiterate that I really appreciate your work! This is just a thing I think is good to consider to make the crate even better 😁

I would be more than glad to do the migration to anyhow myself if I get the approval.

@nathanbabcock
Copy link
Owner

Hey @odilf!

I understand that this project has a "no dependency" rule. I don't know if that's a hard rule for you, but I would consider changing it.

I suppose I would not call it a "hard rule", maybe just a personal aesthetic preference for minimalism. The points you make in favor of anyhow are compelling, though. In particular I think it is a problem if you can't use the two libraries together without jumping through extra hoops. I would say that ease-of-use is the most important goal. And you are right that the error module was inspired by a stripped-down version of anyhow, anyways.

I would be more than glad to do the migration to anyhow myself if I get the approval.

If you have a good idea of what this refactor would look like, then go for it — and I will help review & test. Ideally it would be backwards compatible, but it's not the end of the world if that isn't possible. Hopefully the introduction of anyhow would make this crate even easier to use.

(And if anyone stumbling across this issue disagrees with that, make it known!)

@andresv
Copy link

andresv commented Feb 7, 2024

thiserror is typically used for lib crates. However sidecar seems to be somewhere in the middle.

From thiserror docs:

Comparison to anyhow:

Use thiserror if you care about designing your own dedicated error type(s) so that the caller receives exactly the information that you choose in the event of failure. This most often applies to library-like code. Use Anyhow if you don't care what error type your functions return, you just want it to be easy. This is common in application-like code.

@odilf
Copy link
Contributor Author

odilf commented Feb 7, 2024

That sounds great!

Ideally it would be backwards compatible, but it's not the end of the world if that isn't possible.

So... I don't think there is a way to make it strictly backwards compatible. The Error struct in this crate has two public fields that anyhow::Error just does not have. However, in practice I suspect it's going to be almost always compatible, because people usually just use the error to unwrap, expect or display. I think it should be fine.

Maybe we want to keep the error module tho? I.e., make it re-export the anyhow Error so people using that module have less things to change. I'd rather remove it too since it's a breaking change anyway, but I don't have strong feelings about it.

@odilf
Copy link
Contributor Author

odilf commented Feb 7, 2024

thiserror is typically used for lib crates. However sidecar seems to be somewhere in the middle.

Well, I would say thiserror is better when you can have structure to your errors and you're able to enumerate all the failure modes. Here we forward various ffmpeg errors which are unstructured so might as well just use anyhow, imo.

Also, using anyhow is a small change because as @nathanbabcock said the current Error is very similar to anyhow::Error. Using thiserror would entail more knowledge of the codebase and more design decisions. If we wanted to use thiserror, I think it should be a separate issue.

Oh and lastly I believe a lot (if not most) downstream consumers would/are using anyhow regardless, so it's kinda the same either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants