tokio-rs / valuable

MIT License
187 stars 20 forks source link

Implement `Valuable` and `Enumerable` for `Result<T, E>` #69

Closed hawkw closed 2 years ago

hawkw commented 3 years ago

Closes #18

This PR adds Valuable and Enumerable implementations for Result<T, E> where T and E are Valuable.

@carllerche's comment in https://github.com/tokio-rs/valuable/issues/18#issue-890512640 suggested that Results should "flatten" the inner values rather than implementing Enumerable.

However, this PR does implement Enumerable. This is because simply flattening Results to the Ok or Err value would make it impossible for the Visitor to determine if a given Result is an Ok or Err value, which seems quite important for some use cases. As I described in https://github.com/tokio-rs/valuable/issues/18#issuecomment-938905149:

If the Err side of a Result is not a &(dyn std::error::Err + static), there's no way for the visitor to know if the value is an Ok or an Err.

As a (somewhat contrived) example, if I recorded a Result<usize, usize>, the visitor would totally lose the information of whether the Result was an Ok or an Err...which may actually be the only information that we actually want to record.

Similarly, in some cases, Result<T, ()> is a perfectly reasonable return type.

IMO, Results should be Enumerable, so that the information of whether it was Ok or Err is always retained. But, I could probably be convinced otherwise by a sufficiently good argument...

If others disagree that this is the right approach, I'd be happy to discuss it further, but I thought I'd go ahead and open a PR in the meantime. :)

hawkw commented 3 years ago

Thanks for the review @taiki-e. I'd like to get @carllerche's opinion on this as well before merging, since it's different from how he originally proposed implementing Valuable for results.

hawkw commented 2 years ago

ping @carllerche, any thoughts on this?