Use thiserror crate to make error definition more ergonomic.

[wt]

The thiserror crate takes care of a bunch of details of implementing errors for a library in rust. This will simplify the code around our errors by making it follow conventions that are widely used in the rust community. It also is much shorter in terms of lines of code.

Closes #638

[wt]

It should be noted that this is currently a draft as it only changes the main error type and not the other errors.

FWIW, I really like how it makes the code more concise. Even more of this may be possible since it can also handle common cases for implementing the From trait.

I will clean up the msrv related stuff before any kind of land. I believe that problem may be fixed by fixing #639.

[Mingun]

Thanks for a PR!

As I explained in #638 (comment), I would not merge it right now, because new dependency increases MSRV and does not give any significant improvement, but you a free to continue working on it and add such improvements

[Mingun]

Probably this could have transparent attribute

[Mingun]

Misprint

Suggested change

	#[error("Uknonwn namespace prefix '{:?}'", crate::utils::Bytes(.0))]
	#[error("Unknown namespace prefix '{:?}'", crate::utils::Bytes(.0))]

[dralley]

As I explained in #638 (comment), I would not merge it right now, because new dependency increases MSRV and does not give any significant improvement, but you a free to continue working on it and add such improvements

@Mingun It should be noted that thiserror only requires an MSRV of 1.56, not 1.60. MSRV 1.56 is aligned with several other popular crates such as the latest version of syn, which most will likely have somewhere in their dependency tree.

1.56 was released in October 2021 and is the "Rust 2021 edition" release. I personally don't have any objections with upgrading our MSRV to that.

Whether or not to include thiserror, I'm weakly in favor, I agree that it's not a huge improvement but I do appreciate the ergonomics of defining error messages alongside the variants, and I use thiserror in several other crates.

[wt]

@Mingun what would be the timeline for moving to an MSRV of 1.56?

FWIW, 1.56 was released on Oct 21, 2021.

[Mingun]

I think as soon as we would need to use some feature that is available only since 1.56. I think, that adding a new dependency, that increases MSRV and compile time only to save few lines of code is not a big win, but some user could get upset. So I think we should wait until thiserror helps us to implement a really useful feature. As I noted before, this feature could be a backtrace capturing if capturing backtrace requires to do something special instead of getting it automatically by free.

Also, investigating MSRV of our primary users also would be a good indicator whether it makes sense to increase it in near future or not.

[dralley]

Also, investigating MSRV of our primary users also would be a good indicator whether it makes sense to increase it in near future or not.

serde_derive has an MSRV of 1.56 https://github.com/serde-rs/serde/blob/master/serde_derive/Cargo.toml

So at a minimum that means everyone using the serialize feature is on 1.56+. Our MSRV CI lint only runs cargo check without --all-features, which is perhaps an issue.

[wt]

I think it's important to define a time period that MSRV is trying to capture. In my mind, two years of rust releases seems like a good place to be right now. Do you agree?

[dralley]

@Mingun As noted in my previous comment I think for most of our users it is possible that our practical MSRV is in fact already 1.56 due to serde_derive

[wt]

@dralley FWIW, I was trying to get consensus on the MSRV's underlying purpose. I agree with your analysis.

[dralley]

So the MSRV is bumped. Are we yes or no on this PR? (if yes, it needs a rebase)

[Mingun]

Not right now. I think, we should not introduce new dependences just because we can. As I already mentioned, if this PR will introduce some useful feature that we doesn't have right now (for example, backtraces, I found that it is difficult to understand where the parser error occurs when I examining some bug reports. Again, I'm not sure that backtraces need some special support from error types, but we don't have them right now when tests fails, so I think, that backtraces should be explicitly requested. That question needs investigation), then I merge it. But if it just changes the way of error declaration, it not a reasonable reason to have it.