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

Chores: Edition 2018, quick-error -> thiserror, deps update #15

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

drahnr
Copy link

@drahnr drahnr commented Nov 18, 2020

A bunch of chores, should be fairly well separated into individual commits.

@drahnr
Copy link
Author

drahnr commented Nov 27, 2020

Hey @tailhook, is there anything left to do here?

@drahnr
Copy link
Author

drahnr commented Dec 19, 2020

☃️ it'd be awesome if I could get some feedback on this 🙂

@tailhook
Copy link
Owner

Yes, sorry. This has so much stuff, I don't have enough time to review. I'm also not sure if I agree with thiserror and fs_err. This crate has their own errors to have both, nice error messages or performance on failure path if needed.

But I still need to look at details, which I had no time to do yet. Hopefully, I'll find some time to take a look soon.

@drahnr
Copy link
Author

drahnr commented Jan 21, 2021

thiserror provides a zero overhead and zero extra weight, it's just convenience for error definition. fs_err adds path information to std::io::Error messages and does not introduce an extra error type iirc. So both are effectively no interface changes.

The rest is mostly chore.

The thing that is breaking, are the additional bounds on Box<dyn Explainable> which is now Box<dyn Explainable + Send + Sync + 'static, but I am not 100% sure about that change being needed, it was needed for the use case though I had, but that resulted in this rustc issue rust-lang/rust#78700

@drahnr
Copy link
Author

drahnr commented Feb 20, 2021

Anything I can add or do here?

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 this pull request may close these issues.

2 participants