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

Switch from thiserror to displaydoc #860

Closed
Manishearth opened this issue Jul 15, 2021 · 2 comments · Fixed by #863
Closed

Switch from thiserror to displaydoc #860

Manishearth opened this issue Jul 15, 2021 · 2 comments · Fixed by #863
Assignees
Labels
C-meta Component: Relating to ICU4X as a whole S-small Size: One afternoon (small bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Milestone

Comments

@Manishearth
Copy link
Member

Part of #812

We currently use thiserror, which works like this:

#[derive(Error)]
enum Foo {
   #[error("something {0} something {1}")]
    One(u8, String),
   #[error("something {0}")]
    Two(String)
}

Unfortunately, thiserror isn't no_std, and while there's discussion for getting it there, it's been stalled for a while with some tricky concerns

The main thing we get out of thiserror is Display impls. The std::error::Error impl is largely unimportant, and in our case we can do a blank impl to proxy to Display if we have Display impls.

A crate that does this just for Display is displaydoc. It's inspired by thiserror, and works like this:

#[derive(Display)]
enum Foo {
    /// something {0} something {1}
    One(u8, String),
    /// something {0}
    Two(String)
}

We should switch.

@Manishearth Manishearth added the S-small Size: One afternoon (small bug fix or enhancement) label Jul 15, 2021
@sffc sffc added C-meta Component: Relating to ICU4X as a whole T-techdebt Type: ICU4X code health and tech debt labels Jul 15, 2021
@sffc sffc added this to the 2021 Q3-m1 milestone Jul 15, 2021
@Manishearth
Copy link
Member Author

A couple issues that might crop up:

  • thiserror gives us #[from], which we'll lose out on. We can write these manually (it's not much work), or write our own custom derive for this
  • we use Box<dyn Error> in a bunch of places. These APIs are incompatible with no_std. We can:
    • Use core-error. This keeps us compatible with std::error when using std and otherwise works on no_std
    • Just use dyn Display. I prefer this one: ultimately std::error isn't used that much and the error handling working group is working towards revamping it, so the community doesn't really focus on it as much.

@Manishearth
Copy link
Member Author

I realized that we actually already use doc comments on our errors and they're nicer than the error messages themselves, so I added a feature to displaydoc where we can use an attribute instead when we need to: yaahc/displaydoc#31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-meta Component: Relating to ICU4X as a whole S-small Size: One afternoon (small bug fix or enhancement) T-techdebt Type: ICU4X code health and tech debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants