search

ostrosco / comms-rs

A library for building DSP communication pipelines.
MIT License
13 stars 1 forks source link

Adding Result type to call function in Node trait #59

Closed ostrosco closed 5 years ago

ostrosco commented 5 years ago

This is the first stage of adding error handling into the current node architecture. The biggest change is that the call function in the Node trait now returns a Result<T, Error> with Error being derived from the failure crate. This doesn't improve the error handling in any way; this merely gives us the ability to handle errors from our functions at all.

Rust's type system can't resolve the return type of anonymous closures that get used in call. For the nodes where the closure simply calls another function, return type annotations are not required. For anonymous closures that do work (mostly seen in our unit tests), return type annotations are now required.

ostrosco commented 5 years ago

I guess the main factor would be where we want to handle error translation. At some point, we need to be able to take arbitrary errors and translate them to our error types for deciding exactly what to do. Should the error translation happen in the function being called? Or should it happen after the function call? I can see benefits on both sides.

If we have error translation outside the function to be called, authors writing their own functions don't need to worry as much about error handling. They can just use ? to shortcut out of their function. However, this forces a dependency on failure or some similar crate and if the user wants to handle or interpret errors differently, they still have to do it but in some yet undefined function that would require implementation.

If we have error translation in the function to be called, most of the error handling ergonomics disappear unless the user starts implementing From for everything, which I think is a bad idea since different nodes may interpret the same error differently. However, then we can drop failure and the error translation is all in one spot and we force the implementer to think more about what should happen based on particular errors.

My gut says that error ergonomics are more important at this stage so failure::Error makes sense, but I'm definitely open to change if the consensus thinks otherwise.

ostrosco commented 5 years ago

After doing some more research into the topic, I agree that the right approach is to use our own error types. Thanks for the feedback, @Alex-Addy!

Alex-Addy commented 5 years ago

I like this more. My only concern is that there is no way to store a cause inside the error. Can we change the enums later to carry a cause without changing too much?

Also, currently it appears that the Node will just panic on any error. Will adding behavior based upon the enum be a separate PR?

ostrosco commented 5 years ago

Yes, adding behavior based on the enum will be a separate PR. This PR is just to encapsulate the large change of updating the call function to expect a Result so the PR can be reasonably small. Adding stuff into the enum would be super low impact. I think we'd need to have a discussion about what we want to put in the errors, along with more discussion about what error states we want and need to put into NodeError.