jpernst / rental

Rust macro to generate self-referential structs
Apache License 2.0
211 stars 11 forks source link

Need more examples for TryNewError #21

Closed spease closed 6 years ago

spease commented 6 years ago

It's unclear how to use Trynewerror with an existing error handling paradigm (in this case an enum-based approach). I'm blocked on e0210 trying to create a from implementation to convert trynewresult into my crate's result type.

jpernst commented 6 years ago

Yeah, this is a little tricky. The reason TryNewError exists at all is so that you can recover the head field in the event of failure, otherwise it would simply be consumed and lost, which may not be acceptable. If you don't care about recovering the head, you could use the map_err method on Result to extract just the meaningful error part and discard the head field.

I could add a blanket impl on my end to just allow TryNewError to convert into anything that its underlying error type can convert into. I didn't do this so far because it felt like it would make it too easy to silently lose the head field, but I suppose that in cases where you want to just return on error, the loss of the head is probably what people would expect to happen anyway. Is this the behavior that you'd like to see, or perhaps something else?

Perhaps the new failure crate could help as well, but I want to give that a little more time to shake out before I commit to adding it to the API.

spease commented 6 years ago

That does sound too broad. Can you some way to automatically convert from trynewresult to Result<T,E2> types where E2: From?

This would still require an explicit trait definition to throw away the H. However I'm guessing you might run into the same error that I was.

I eventually got past it by manually map_err() and annotating the error type st trynewerror is taking my custom error type. Took me 20-30 minutes to figure out though.

On Nov 23, 2017, at 11:55, jpernst notifications@github.com wrote:

Yeah, this is a little tricky. The reason TryNewError exists at all is so that you can recover the head field in the event of failure, otherwise it would simply be consumed and lost, which may not be acceptable. If you don't care about recovering the head, you could use the map_err method on Result to extract just the meaningful error part and discard the head field.

I could add a blanket impl on my end to just allow TryNewError to convert into anything that its underlying error type can convert into. I didn't do this so far because it felt like it would make it too easy to silently lose the head field, but I suppose that in cases where you want to just return on error, the loss of the head is probably what people would expect to happen anyway. Is this the behavior that you'd like to see, or perhaps something else?

Perhaps the new failure crate could help as well, but I want to give that a little more time to shake out before I commit to adding it to the API.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

jpernst commented 6 years ago

Another possibility I've been considering is to just have a try_new_or_discard or somesuch method that will return a normal result without the head, for when you know you don't need it. Would you find that useful or are you satisfied with the map_err solution as it stands now?

jpernst commented 6 years ago

I've been meaning to push an update to add the readme to crates.io anyway, so I went ahead and added this new convenience method as try_new_or_drop. Hopefully this makes interacting with existing error handling strategies a little easier.

spease commented 6 years ago

My concern would be that this makes the API more complex, and there already doesn't seem to be any direct documentation of it (it looks like you're expected to go through examples to understand it).

I already feel like I'm stumbling around in the dark (when do I use rental vs rental_mut? Why does rent_all go away when I specify the latter?)