Skip to content

Conversation

@BrknRobot
Copy link
Contributor

This allows code which can handle any type of promise. Creating IPromiseBase allows IPromise, IPromise<int>, and IPromise<MyFunClass> to all be treated identically, without creating code branches for each. The IPromiseBase interface is implemented explicitly, which avoids creating ambiguous functions.

Additionally, the Promise_Base class allows for a decent amount of code reuse between generic and non-generic promises.

This allows code which can handle any type of promise. Creating IPromiseBase allows IPromise, IPromise<int>, and IPromise<MyFunClass> to all be treated identically, without creating code branches for each. The IPromiseBase interface is implemented explicitly, which avoids creating ambiguous functions.

Additionally, the Promise_Base class allows for a decent amount of code reuse between generic and non-generic promises.
@BrknRobot BrknRobot mentioned this pull request Mar 17, 2017
@RoryDungan
Copy link
Contributor

RoryDungan commented Mar 20, 2017

This looks great - would definitely be a good addition to the library. There are couple of things that would be good to change before we merge it though.

Some of the new methods don't have <summary> comments, which are important to add even if they're copy-pasted from the interface or another overload of the same method just to ensure that the help comes up in Visual Studio when you're writing code that references the method.

It would also be good to have a couple of extra tests to test chaining different types of promise together, now that that is possible.

@BrknRobot
Copy link
Contributor Author

Writing tests made me realize I missed some functionality. I'll have to implement a few more things before this is ready.

@RoryDungan
Copy link
Contributor

That's fine. Let us know when this pull request is ready to merge and we'll take another look at it.

@sindrijo
Copy link
Contributor

What is the status of this @nloewen? This would be a very nice addition, just asking before I implement this myself. What functionality was missing?

@BrknRobot
Copy link
Contributor Author

It's unlikely I'll get around to completing this. I don't recall what functionality was missing. Most likely some combination of chained promises wasn't supported. If you start adding test cases, I'm sure you'll find it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants