failure -> thiserror/anyhow #220
Conversation
This commit migrates error handling with failure to thiserror and/or anyhow. This is mostly good because (1) the wider Rust community uses thiserror/anyhow for error handling, (2) failure has been deprecated, (3) GitHub occasionally sends scary emails that there are security risks associated with using failure. I've tried to be thorough in the conversion, but ultimately how things should look is quite opinionated. Internal tests pass, and my own code works with the updated casatables code. A lot of things are drop-in replacements, other things needed a bit more work, like setting up new error types. This commit also removes the ctry! macro. The concept provided by this macro is pretty much the same as using anyhow::Error as function return errors and annotating things that can fail with "with_context".
Add the column/field name to the end of the message.
This is important because some dependency versions don't actually work, e.g. thiserror = "^1.0" allows 1.0.0, but actually >=1.0.7 is what is required for rubbl to compile. This commit also removes all of the "^" versions because (IMHO) it's clearer to use the major.minor.patch layout and it does the same thing. This commit removes version ranges on ndarray and num-complex (I introduced these in the first place, thinking they were a good idea). Why are version ranges bad? At the time of writing, 0.15.6 is the latest version of ndarray. If my code depends on ndarray 0.15.0 and rubbl depends on ndarray >= 0.8, cargo will determine that ndarray 0.15.6 is fit for purpose. But, if tomorrow ndarray releases 0.16.0, then cargo will inspect these requirements and determine that 0.16.0 is best for rubbl and 0.15.6 is best for my code. Now I depend on two versions of ndarray, and I can't interact my ndarray code with rubbl's ndarray code. This is a huge pain, so it's (IHMO) simpler to have libraries (like rubbl) depend on their oldest semver compatible versions, i.e. today that is 0.15.0. If/when ndarray 0.16.0 is released, then others depending on ndarray 0.15.0 and rubbl won't have their code break, and rubbl should be eventually updated to support ndarray 0.16.0, allowing rubbl consumers to also update. clap is a bit of an exception; 4.0.0 probably works, but clap is mostly intended for binary usage, not library usage, so I think it makes the most sense to enforce a modern version of clap with lots of bug fixes.
Make anyhow, clap and termcolor optional dependencies in rubbl_core; they are now activated by the "notifications" feature. This means that downstream consumers of e.g. rubbl_casacore don't pull these dependencies in which aren't actually used in the library. Remove approx as a dependency. This was only in the hope of reducing friction between date-type-exporting crates like num-complex, but, (now that we all understand this a bit better,) this doesn't do anything here.
|
Thanks once again for putting in the work on this! I hope/aspire to review this swiftly, and with my release automation tooling it will be easy to put out new releases after merge. As for backtraces, I think that the backtrace API was just stabilized in the most recent Rust release, after being unstable for a very long time, so now they should be available everywhere that's running the most recent compilers. |
Thank you. Please take your time; there's no huge rush here.
Ah, cool. I'll try to remember to update the relevant MSRVs in the Cargo.toml files. |
|
So I just had a quick look at when backtraces were made stable (apparently the most recent version, 1.65) and adding this minimum supported Rust version (MSRV) to the |
I'm pretty sure that the traversal logic won't add anything in these specific examples, but I'd like the example code to show the thorough way of doing things.
The latter is more concise.
... with some updated and expanded documentation. I think it provides nice syntactic sugar.
|
OK, well my aspiration for prompt review did not come to pass. But after being reminded I have now reviewed this! Thanks again for taking on this effort and doing a nice job with this somewhat tedious work. I've pushed some updates to your branch:
As a last item, I am not sure if the updates to the dependency version specifications are doing exactly what's described in the commit message. I like the caret syntax rather than sigil-less syntax, but that's not a big deal either way, and boosting the minimum versions of various dependencies to be accurate is good. But for the ndarray minimum versioning, I don't quite follow that the change is helpful. If Rubbl's dependency is (Based on all of the above I think you already know this, but I'll mention: one thing that tripped me up for a while is that in Cargo's semver implementation, a dep on a pre-1.0 version like |
|
Thank you for your time, @pkgw. I've had a look at your changes and I have no opinions on it, so I think things are good to go. As for the
It is possible to fix this after Cargo's automatically resolved dep versions by running So as much as I dislike the situation, I think the lesser evil is to have breaking releases of the |
|
@cjordan OK, I'd like to try digging into the Cargo situation a little bit more. My read of the documentation is that what you're describing shouldn't be happening; maybe we have something funny about our situation. Or maybe I'm misreading the docs and/or they're inaccurate. |
|
Tried posting to the Rust forum: https://users.rust-lang.org/t/cargo-duplicating-dependency-when-it-seems-like-it-shouldnt-be/87883 |
|
OK, I dug into this a bit more; see links in the above forum thread, which basically resolve to rust-lang/cargo#9029 . The TLDR is that the behavior you've identified is indeed Just How It Is for now, and while there is some interest in improving that, nothing's going to be changing soon. Given that I think your change is the best we can do, but I'll add some stuff to the README(s) to hopefully steer future people in the right direction. |
|
Thank you for time here, checking, testing, merging and spending time rousing the community of the issue. I know there's work being done w.r.t. making casatables faster, but could a new release with these changes be cut, please? Hopefully any improvements that come out of the other work could manifest as patch versions as probably dont break semver. |
... and a bunch of other small changes.
The commits themselves are well commented in terms of their purposes, but basically this work removes error handling with failure, instead using thiserror/anyhow, which is in widespread use in the Rust community.
It seems GitHub hasn't recognised that this is the same PR I submitted about 18 months ago; here's @pkgw's comment from that time: #148 (comment)
I'm not sure I understand all the questions from the comment made on the previous PR, but I'll attempt to explain my thinking here. Using enums instead of structs as error types means that functions can return a "family" of errors rather than one specific error. It is possible to have a error struct contain a
Stringand just pass that around, with the string being a human-readable message, and there's nothing necessarily wrong with this (indeed I think there's no single right way to do errors). IMO it is better to have enums because then the errors can be typed, and if the caller wants to, the error variants can be matched and handled. I guess this is also possible with the string approach, but this is much looser than using enum variants.FWIW I leverage the typed error approach heavily here.
In most cases I expect that users don't have the time/effort to do this kind of fancy error handling, and because most errors are unrecoverable (e.g. your code wants to open MS table "DATA", but it isn't there, what else could you possibly do?), the
?should work as one expects it to.All this said, I have been lazy in my porting of the
miriadcode, where I just dump existing string errors into a catchallGenericerror enum variant; this could obviously be expanded depending on how much you/others care.In terms of extensibility; I think semver compatibility is something every library maintainer should consider high priority, regardless of how errors are handled, so adding new variants to an existing error enum should be considered as a breaking change, requiring a new release of a crate. If the "stringy" approach were employed instead, obviously more errors could be stuffed into a catchall variant, but I'd be careful about pulling the rug out from users attempting to handle error variants, even if they're not typed.
Finally, I'm not sure if
anyhowsupported backtraces at the time of my last PR, but it appears to do so now. I haven't tested them or even used them, but the code compiles, so 🤷If/when this PR is merged, it would be great to have a new release of rubbl and it's family of crates. Thanks once again for your time!