Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create base class for promises #35

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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