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

Improve Interoperability with Error Reporting Tools #355

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

Conversation

dtoniolo
Copy link

Introduction

I recently opened an issue (#354) thinking that there was a bug in this crate, but the problem that I was having tourned out to be caused by a bug from serde. While I was going through the source code of this crate I noticed a minor thing that could, at least according to my personal opinion, be improved. Given the great value that I was able to get from this library, I thought I could attempt to give a small contribution to rust-csv and implemented this PR.

Before describing my proposal, I'd just like to clarify that although I opened a PR, I don't think that the code I'm presenting is necessarily ready to be merged yet. I've submitted it just so that we discuss the changes while looking at something concrete. The code compiles and passes all the tests.

Problem Description

The issue I'm having is with the way in which errors from this crate are formatted in their Display trait. In particular, if any csv::Error has a source error, it will be formatted as "<error description>: <source description>". The resulting string is hard to read, especially when the error chain is long.

Chaining errors descriptions by placing them in the same string and separating them with a colon is a simple, common practice. I was doing the same until a few months ago, when I've come across anyhow and eyre. Now I use them to format my errors and the legibility of my logs has greatly improven as a consequence.

Unfortunately, csv::Error currently doesn't play nicely with these tools. The first reason is, of course, that it manually handles the formatting of its source. The second is that it doesn't implement std::error::Error::source, so that eyre isn't able to follow the error trace and print it.

Proposed Solution

This PR would fix both of the issues I've just described.

  • First of all, it adds an implementation for std::error::Error::source, which would improve the interoperability with error reporting tools I've just described. It would also allow any client code to use std::error::Error::source and would better model the semantics of the errors that rust-csv produces.

  • Secondly, it changes the implementation of Display for csv::Error so that the resulting string doesn't include the source error anymore, leaving this task to eyre and similar libraries.

  • I've also updated the tutorial so that now most of the example report their errors with eyre. I've done it in order to be proactive, but it's the change I'm less confident about: on one hand readers might not want to bother eyre, but on the other if they were to run the examples without it they wouldn't be getting a complete description of the error, which might be confusing. Moreover, one of the tests associated with the tutorial relies on the description of a nested error being present, so it would break if eyre were not used.

Finally, I've not added eyre to the cookbook, because it didn't seem worth it, but I can fix this if you prefer.

by implementing the `::std::error::Error::source` method and ensuring
that `::std::fmt::Display` prints only the description associated with
the top level error, leaving the rest to the error reporting tools.
During the previous commit some code snippets hadn't been updated to
match their corresponding `.rs` files.
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.

1 participant