-
Notifications
You must be signed in to change notification settings - Fork 24
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
Should we have a derive for Display? #11
Comments
I think there's definitely value in addressing this. In particular one crate that seems worth looking at is #[derive(Debug, Snafu)]
enum Error {
#[snafu(display("Could not open config from {}: {}", filename.display(), source))]
OpenConfig {
filename: PathBuf,
source: std::io::Error,
},
#[snafu(display("Could not save config to {}: {}", filename.display(), source))]
SaveConfig {
filename: PathBuf,
source: std::io::Error,
},
#[snafu(display("The user id {} is invalid", user_id))]
UserIdInvalid { user_id: i32, backtrace: Backtrace },
} |
There's not a big difference between that and using our Error derive and the Display derive in the other crate I wrote, which I believe should just work like this (but I haven't revisited the display derive in a while so I may be a little wrong): #[derive(Debug, Display, Error)]
enum Error {
#[display("Could not open config from {}: {}", filename.display(), source)]
OpenConfig {
filename: PathBuf,
#[error(source)]
source: std::io::Error,
},
#[display("Could not save config to {}: {}", filename.display(), source)]
SaveConfig {
filename: PathBuf,
#[error(source)]
source: std::io::Error,
},
#[display("The user id {} is invalid", user_id)]
UserIdInvalid { user_id: i32, backtrace: Backtrace },
} But I'm considering the alternative of having a display that hitches a ride on doc comments instead: #[derive(Debug, Display, Error)]
enum Error {
/// Could not open config from {filename.display()}: {source}
OpenConfig {
filename: PathBuf,
#[error(source)]
source: std::io::Error,
},
/// Could not save config to {filename.display()}: {source}
SaveConfig {
filename: PathBuf,
#[error(source)]
source: std::io::Error,
},
/// The user id {user_id} is invalid
UserIdInvalid { user_id: i32, backtrace: Backtrace },
} However, I'm also inclined to just do nothing in this crate, since you can depend on another crate to provide a derive for Display according to your preferences. |
I think keeping this small and specialized, makes sense. We have many options to easy the |
If there are other crates that do the job well, I think it makes sense to let those crates do it, but definitely put some options for it in the documentation so that users don't have to go looking for them themselves. |
I like the Idea of using doc comments, however there should be a way of specifying the display string explicitly, too. Thus, generating Additionally, using docs comments should look pretty on both sides: the Docs, and in In either case you are right, this probably shouldn't be implemented in this crate anyway. |
I personally like the idea of a dedicated crate focusing on this problem so it is "friendlier" for people to pull in for other uses. I find the idea of using comments intriguing, similar to how structopt does it but with the need for string interpolation. I think the main question is how often the doc comments are needed for documentation or not and if this will impact the documentation too negatively. For me, I think I've copy/pasted a lot between doc comments and |
I'm inclined to agree with everyone that I will not re-export a Display impl from this crate, but instead recommend other crates that users can use. |
Implementing Display is actually the most annoying part of defining your own error. Should der Fehler provide a derive to help you with this?
One option is one of the relatively "obvious" less opinionated derives, like the one found in the display derive crate I wrote for failure. Another option is to do something fancy like deriving the display from doc comments.
The text was updated successfully, but these errors were encountered: