search

thruster-rs / Thruster

A fast, middleware based, web framework written in Rust
MIT License
1.06k stars 47 forks source link

Example error handling #113

Closed kjvalencik closed 5 years ago

kjvalencik commented 5 years ago

To help with complex error handling on ThrusterError I added some convenience methods for downcasting the cause.

An additional proposal is to implement StdError for ThrusterError with fn cause returning the cause.

Thoughts?

trezm commented 5 years ago

Also -- forgot to mention, we should definitely implement Error for ThrusterError.

kjvalencik commented 5 years ago

I can add that into this PR. However, if ThrusterError implements Error I might need to think a bit harder about how that plays with downcast since the Error trait already implements that.

I'll think on this a bit more. Thanks!

kjvalencik commented 5 years ago

Now that I think about it, I don't think the downcast is going to conflict because typically you would only be calling Error::downcast when you have a dyn Error. It doesn't make sense to call it when you already have a ThrusterError. If for some reason you really need to, there's always UFCS.

trezm commented 5 years ago

I'm wondering if instead of adding those methods to ThrusterError if we should just drop that whole part and implement Error on ThrusterError instead.

kjvalencik commented 5 years ago

These methods aren't exactly analogous to Error::downcast. Those downcast the error itself, where these downcast the cause. You would write similar code except instead of self.cause... it would be err.cause().

This code is convenient at the moment due to returning ThrusterError. I wouldn't recommend it if async-core was generic over the error type.

Honestly, I'm very torn on this code and haven't convinced myself that it should be part of thruster. Everything done here can be done in the application.

I think the example is valuable in either case. What is your preference? Would you like to keep this code and split out the example or would you like to remove this code and move it into an example?

trezm commented 5 years ago

These methods aren't exactly analogous to Error::downcast. Those downcast the error itself, where these downcast the cause. You would write similar code except instead of self.cause... it would be err.cause().

Mm, yes, I see that now, in that case I recommend changing the names to make that more explicit. It's not a big leap to assume that they would do the same thing.

Honestly, I'm very torn on this code and haven't convinced myself that it should be part of thruster. Everything done here can be done in the application.

I think the example is valuable in either case. What is your preference? Would you like to keep this code and split out the example or would you like to remove this code and move it into an example?

I think you're dead on -- the example is definitely valuable. Let's keep the example, separating it out like we were saying earlier so that basic stays basic, and drop the implementation in the actual library until we can see how much of a pain point this is.

In my opinion it's better to have a third party middleware that everyone uses and later incorporate it, then potentially confusing or unused functionality.

kjvalencik commented 5 years ago

Updated the PR to remove the changes to thruster and provide a downcast example.