chore: Better error types for TreeBuilder #563

I read up a little about the best practices for error handling in Rust

Since you mentioned it, here's a nice blog post I recently saw on this topic.

if it had a sort of constructor, then the panicking could be done there

yes; for that, you could have a struct for that particular error variant, put the panicking logic into its constructor as you say, and impl From<ThatErrorStruct> for TreeBuildingError so that you could do the following in the place you create and then return it with a ?:

if something_bad_happened() {
    Err(ThatErrorStruct::new(param))?;
}

But I don't really think we'll be throwing the same error from different places all that often.. so I'd go with a cfg!-d panic! approach, and I think you can avoid unreachable_code warnings by doing the following:

if something_bad_happened() {
    if cfg!(debug_assertions) {
        panic!("some message")
    } else {
       Err(ThatErrorStruct::new(param))?;
    }
}

It's of course unfortunate that that'll require us to repeat the error message (once in the #[error()] of the enum variant and once in the panic!), so maybe we could something like this instead?

if something_bad_happened() {
    let err = Err(ThatErrorStruct::new(param).into());
    if cfg!(debug_assertions) {
        err.unwrap();
    } else {
       return err;
    }
}

Not saying that this is pretty of course...

Since you mentioned it, here's a nice blog post I recently saw on this topic.

That's indeed one that I enjoyed reading! I didn't go for the recommended solution of having an enum in a struct, because I felt this was mostly motivated by API stability considerations which don't apply to us as we don't intend to maintain a stable Rust API (so far).

so I'd go with a cfg!-d panic! approach

Makes sense! I didn't know about cfg!, it's a good solution.

That's indeed one that I enjoyed reading!

I guess we're following the same people on Mastodon:)

I didn't go for the recommended solution of having an enum in a struct, because I felt this was mostly motivated by API stability considerations which don't apply to us as we don't intend to maintain a stable Rust API

Yup, fair enough