Done adding Catch with return of Promise

[mystikxkemix]

No description provided.

[RoryDungan]

Thanks, looks like a useful addition but won't work in its current state and needs tests written for both the new functions before we can merge.

[RoryDungan]

This needs the <summary>...</summary> comment on it so that a description of how the method works shows up in IntelliSense.

This should also return IPromise<PromisedT>, not IPromise<ConvertedT>. This is because Catch doesn't always run, so if we try to convert the type of the promise in the catch handler, we have no way of knowing after the catch whether the type is the original PromisedT or the ConvertedT.

promiseReturningAnInt()
    .Then<int>(value => value + 1) 
    .Catch<string>(ex => Promise.Resolved<string>(ex.Message))
    .Then<???>(arg => /* no way to know whether arg should be an int or a string */);

The only case where you can return a different type of promise in the rejected handler is in the overload of .Then where both the resolved and rejected handlers are guarenteed to return the same type - see Promise.cs line 75

[RoryDungan]

Same as above

[mystikxkemix]

I made it like this for my use. But I didn't know how to do it correctly, that's why I made a pull request. Maybe you can help me doing this

[mystikxkemix]

I made a change in my code, the Catch can retrun a promise of the same type of incoming promise. But there is a issue in one of your test that fail the build. I don't know how you want to handle it.

[ReneB]

Hi @RoryDungan!
Since the Promises/A+ spec is pretty hard to read, I'll pose the question: am I right in assuming that this feature is actually required, as per the combination of point 2.2.7.1 and 2.3.2, to be Promises/A+-compatible?
It's something I've noticed we actually need in one of our projects, so if this is actually the case, I'm willing to work on this pull request if you can tell me what needs to be done - aside from making it mergeable - to get it approved and added to the library.
(As an aside, many thanks for all the hard work on this library, it has helped us out a lot!)

[RoryDungan]

Since promise is resolved (promise.Resolve(1) further down), this Catch block is never executed.

[RoryDungan]

Since the promise is resolved, this catch block is never executed.

[RoryDungan]

it would make more sense here to make the first promise reject and verify that actualValue was set to -1, to verify that the value from a promise returned inside a Catch is coming through.

[RoryDungan]

Hey, sorry letting this branch go stale for so long. Reading through the A+ guide it I agree it would seem like this is needed. I just tested and this functionality does work in standard JS promises so would definitely be good to have.

I think in terms of getting this ready to merge again, there are a few things that would need to be done:

• There's a merge conflict that would need to be resolved
• The implementation is only for the generic promise, but would need to be done for the non-generic one too.
• Two tests have been added but none of them actually test that the promise returned by Catch is getting executed or that the value returned from the promise returned by catch is passed to the next Then. I've made some comments on the code explaining this in more detail.

[ReneB]

I've started to work on it a bit in my own fork, but I have a judgment call to make and I wanted to ask you for your opinion.

If we add an overload for Catch, some existing code will become ambiguous. For example:

promise.Catch(e => throw new Exception("This shouldn't happen"));

can no longer be resolved, since - because the return type needs to be implied since there is no return in the throw-only body - both Catch(Func<Exception, IPromise<PromisedT>> onRejected) and Catch(Func<Exception, PromisedT> onRejected) would suffice - and thus the code won't compile anymore, making this a backwards-incompatible change.

OTOH, I could add it as a method with a signature like public IPromise<PromisedT> Recover(Func<Exception, PromisedT> recoveryHandler) and the change would be backwards-compatible, but there would be another method added and the user would need to know just a bit more about how the library is supposed to be used again.

How do you feel about this?

(Edit: the name Recover could, of course, apply equally well to the version with the X(Func<Exception, PromisedT> onRejected) signature because it also, sort of, allows recovery - but it also terminates the chain immediately - and if we were to rename both, we're sort of back at the beginning.)

[ReneB]

Since (I think) I have no way of updating this pull request, see #109 for a new version of the code that should be mergeable and fixes the aforementioned issues.