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

verbose errors feature #551

Merged
merged 3 commits into from Nov 20, 2019
Merged

Conversation

killercup
Copy link
Contributor

@killercup killercup commented Nov 17, 2019

This adds a new "verbose-errors" feature flag to async-std that enables
wrapping certain errors in structures with more context. As an example,
we use it in fs::File::{open,create} to add the given path to the
error message (something that is lacking in std to annoyance of many).

This adds a new "verbose-errors" feature flag to async-std that enables
wrapping certain errors in structures with more context. As an example,
we use it in `fs::File::{open,create}` to add the given path to the
error message (something that is lacking in std to annoyance of many).
@killercup
Copy link
Contributor Author

killercup commented Nov 17, 2019

We discussed this a while back, and I wanted to provide something small that can be a basis for discussion. The main thing here is to provide an internal mechanism to easily add optional contexts. I hope–but have not proven–that the closure get compiled away in release modes without the feature flag set.

@ghost
Copy link

ghost commented Nov 18, 2019

This is great, thank you! :) I think we don't even need to introduce a new feature flag for this and can just provide verbose errors by default. Is there any real downside to providing this by default?

Wdyt, @yoshuawuyts ?

@killercup
Copy link
Contributor Author

killercup commented Nov 18, 2019

It costs a string allocation in the error case 🤷

@ghost
Copy link

ghost commented Nov 18, 2019

Error cases don't need to be fast :)

Besides, we already have a string allocation for the path, a task allocation for spawn, a push into the blocking task queue, and so on... so the cost of this additional error context is practically zero.

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Overall very excited for this direction, and don't think we need a feature to introduce this.

Left some nits on naming, but otherwise this is great!

src/utils.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@killercup killercup left a comment

Updated: Now without feature flag!

src/fs/file.rs Show resolved Hide resolved
use std::{error::Error as StdError, fmt, io};

#[derive(Debug)]
pub(crate) struct VerboseError {
Copy link
Contributor Author

@killercup killercup Nov 18, 2019

Choose a reason for hiding this comment

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

I'm sure this type could be using source: T but let's refactor this when we have a use case

@killercup killercup requested a review from yoshuawuyts Nov 18, 2019
tests/verbose_errors.rs Outdated Show resolved Hide resolved
@yoshuawuyts yoshuawuyts added this to the 1.1.0 milestone Nov 20, 2019
@yoshuawuyts yoshuawuyts added the enhancement label Nov 20, 2019
@yoshuawuyts yoshuawuyts merged commit 3f8ec5a into async-rs:master Nov 20, 2019
15 checks passed
@killercup killercup deleted the feature/verbose-errors branch Nov 20, 2019
@killercup
Copy link
Contributor Author

killercup commented Nov 20, 2019

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

Successfully merging this pull request may close these issues.

None yet

3 participants