Start Refining Errors

[kazk]

I wanted to add "Refine Errors" (opened #688) to our roadmap, so I tried a bit to see how we can tackle this. Any feedback is appreciated.

Remove impl From<T> for kube::Error

Require explicitly mapping errors.
This commit shows how much we need to mentally process while reading the code (maintenance cost), and how badly grouped they are (usability issue).
Removing impl From forces us to attach context, and this is a good thing. It's verbose right now, but that's because we need to break them down.

As an example, I got rid of kube_core::Error. Parsing group version no longer has an error variant HttpError.

thiserror vs snafu

As I mentioned in thiserror vs snafu discussion, the huge kube::Error is a problem I'd like us to start fixing. I've been using both thiserror and snafu, and I think using thiserror without #[from], and gradually breaking errors down is probably the best approach for kube. While I think snafu help us initially to think of errors in a more scalable way, it's not necessary to implement the pattern. Also, we probably can't do SNAFU way until we finish breaking them down cleanly anyway, and the features to prevent misuse will get in the way (e.g., private context selectors by default). So, I don't think it's worth some downsides.

Stability

thiserror is stable (v1). snafu is still evolving. 0.7 will add Snafu suffix to generated context selectors. Tuple variants are not well-supported? (I haven't used tuple variants with snafu, so I don't know the details).

Popularity

thiserror v1 is used by 9,491 public crates. snafu 0.6 is used by 270. snafu is extra dependency for many.

Features

snafu provides some syntactic sugars, but I don't think it adds that much value. Avoiding #[from] and using the usual Result::map_err feels similar enough, especially as we break the errors down.

let mut file = File::open(path).context(OpenFileSnafu { path })?;
let mut file = File::open(path).map_err(|e| Error::OpenFile(e, path.to_owned()))?;
let mut file = File::open(path).map_err(|source| Error::OpenFile { source, path: path.into() })?;

action().context(ActionSnafu)?;
action().map_err(Error::Action)?;

One feature snafu optionally provides that thiserror doesn't, is backtrace in stable, but this requires the end users to depend on snafu and enable the backtrace feature. There are error reporter crates to do this until backtrace become stable.

Unless I missed something significant, I think we should go with thiserror without #[from].

[nightkr]

Big 👍 on the overall idea.

I've been using both thiserror and snafu, and I think using thiserror without #[from], and gradually breaking errors down is probably the best approach for kube.

That makes sense to me.

While I think snafu help us initially to think of errors in a more scalable way, it's not necessary to implement the pattern.

Definitely, it's all just syntax sugar in the end.

Also, we probably can't do SNAFU way until we finish breaking them down cleanly anyway, and the features to prevent misuse will get in the way (e.g., private context selectors by default). So, I don't think it's worth some downsides.

While it'd be nice to only depend on one of them in the end, we don't need to go all-in from the start. Neither of them are visible in the public API, and they interoperate just fine (aside from SNAFU's backtraces).

thiserror is stable (v1). snafu is still evolving. 0.7 will add Snafu suffix to generated context selectors.

I'm not looking forward to that upgrade, but there's also no huge rush. We're not holding anyone else back from upgrading since we're only exposing the std::error::Error instance anyway.

Tuple variants are not well-supported? (I haven't used tuple variants with snafu, so I don't know the details).

To be honest I kind of dislike tuple variants anyway, since they become quite messy as soon as you want to add any extra context to the errors, as demonstrated by your file example. I'd feel much more comfortable pattern-matching Error::OpenFile { source: io::Error, path: PathBuf } than Error::OpenFile(io::Error, Path) (and this is a relatively tame example since all context has types that more or less indicate what everything is..).

[nightkr]

To be clear: even if I might personally prefer going full SNAFU, (IMO) modularized thiserror is still a vast improvement over what we currently have.

[clux]

Yeah, I like this a lot as well. Gets us going on refining.

thiserror vs snafu

This justification is good. The popularity of thiserror has exploded compared to snafu which is declining (and we are the top listed depender). Thiserror also have dtolnay behind it as a maintainer who is making frequent non-breaking patch releases (.30 now).

Regardless of how pointless a popularity contest may seem, pretty much everyone knows how to wrangle thiserror and that is also nice from a fresh contributor POV.

[kazk]

Cool, I'll open a tracking issue with a summary and task list.

To be honest I kind of dislike tuple variants anyway, since they become quite messy as soon as you want to add any extra context to the errors, as demonstrated by your file example. I'd feel much more comfortable pattern-matching Error::OpenFile { source: io::Error, path: PathBuf } than Error::OpenFile(io::Error, Path) (and this is a relatively tame example since all context has types that more or less indicate what everything is..).

Yeah, we should avoid tuple if it makes the error difficult to use. We should decide what's best for each error, and thiserror doesn't limit us.

[clux]

This looks great to me! Generally just minor nits. But but have one question left on our approach w.r.t. #515 .

[clux]

As an aside here; I think we probably should move our docs to use an async fn wrapper here ala https://github.com/kube-rs/kube-rs/blob/master/kube-runtime/src/events.rs#L134 to reduce the docs cruft everywhere (not necessary in this PR)