Most fallible methods should return Result<T, failure::Error> instead of Result<T, wasm_pack::error::Error>

[fitzgen]

A wasm_pack::error::Error will Into a failure::Error so existing ? locations shouldn't need any changes.

Basically, only "leaf" errors should be wasm_pack::error::Error and everything else should be failure::Error.

Then we should put .context("..") (or .with_context(|e| ...)) onto all non-leaf ? sites, providing better error messages and cause-and-effect chains.

[stuarth]

I'd be happy to take this one!

[stuarth]

@fitzgen wanted to check my understanding on this. The idea is I'll convert a fn like

step_check_crate_config to Result<(), failure::Error> and add a context / with_context to its call to check_crate_config

What I'm not seeing -- and apologies if I'm overlooking something obvious -- is the best way to convert that failure:Error to a wasm_pack::error::Error in a fn like run

Thanks!

[fitzgen]

step_check_crate_config to Result<(), failure::Error> and add a context / with_context to its call to check_crate_config

Correct.

What I'm not seeing -- and apologies if I'm overlooking something obvious -- is the best way to convert that failure:Error to a wasm_pack::error::Error in a fn like run

All those should become failure::Errors too, so no need to convert to wasm_pack::error::Error ;)

Thanks!

Thank you!

[stuarth]

Ah, gotcha. I think I'd misread your intent with leaf / non-leaf.

Am I right in thinking that when you say "leaf" you mean something like a function that reads a config from disk? And a non-leaf fn is one that builds on top of it, e.g. initiating a build that depends on that config?

[stuarth]

Sorry about the inactivity -- life has crept up in a major way and I haven't had a chance to get to this yet. If anyone wants to take a stab at it, please do!

[drager]

I would like to give this a shot. I will try to start working on this soon and create a PR when I have. In the meantime if someone else would like to grab this, please do and let me know if that's the case. :)

[fitzgen]

I ended up doing part of this work in #392 (3rd commit) if you want some examples of what needs to happen here.

I only did enough to give better context to errors when running child processes, so there should be a bunch of places that are still not using failure::Error and a bunch of places taht need .context("...").

[fitzgen]

Oh! And it might make sense to branch off of my PR to avoid a bunch of merge conflict ickiness -- sorry!

[drager]

@fitzgen: Thank you for the information, I will branch out from your PR.