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

Updates #20

Closed
wants to merge 10 commits into from
Closed

Updates #20

wants to merge 10 commits into from

Conversation

BrknRobot
Copy link
Contributor

I've implemented some changes that I needed for my project.

Create unified base class/interface for generic and non-generic promises

This allows code which can handle any type of promise. Creating IPromiseBase allows IPromise, IPromise, and IPromise 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.

Implement Finally

Implemented as described in issue #9. It's just a wrapper around existing functionality.

@df-marc
Copy link

df-marc commented Jul 6, 2016

I'd love to get Finally support in RSG.Promises! If you separated these into two PR's, it might be easier for Ashley to land the Finally support while the team decides which (if any) big refactor PR to take. I hope so anyway.

@mqp
Copy link

mqp commented Oct 19, 2016

👍 for this patch, we use this promise library at AltspaceVR and having a common interface for generic and non-generic promises would be really nice.

@ashleydavis
Copy link
Contributor

Hi Nathan, thanks for contributing.

I'm really happy with some of your changes. Particular the inheritance of generic and non-generic promises from a single base. I think that's worthwhile. I also think that the addition of the Finally function is great and is something that we have wanted for some time.

Generally I'd question the the addition of PromiseResult. Can you let us know more about why this was added? Also I'd prefer to keep formatting as spaces as that's what we use.

We are definitely happy to accept pull requests for the common interface (@mquander) and also for the finally function (@df-marc). However what we need though are two of the simplest imaginable pull requests that each just add the requested feature.

Many people depend on this library (including us at Real Serious Games) so I'm reluctant to change it except for the most important feature additions provided the changes sets are simple as possible (and therefore easiest to prove that they work reliably).

Cheers
Ash

@BrknRobot
Copy link
Contributor Author

PromiseResult was added to support code that works directly with PromiseBase. It allows you to continue the promise chain in a generic fashion without losing access to the promised result. Without this, you need to cast PromiseBase before using it, which is tricky when there arn't any type restrictions on the promises you are handling. Casting to Promise doesn't work like you might hope!

I can fix the formating and split the PR, but I won't have time to work on this until late December.

@mqp
Copy link

mqp commented Nov 1, 2016

No comment on the necessity of PromiseResult, but if it's around it would be nice if it were a struct -- no point in unnecessary heap allocations, and it's common for "value wrapper" classes like Lazy<T>, Nullable<T>, etc. to be structs for this reason.

@BrknRobot
Copy link
Contributor Author

This pull request has been split into #34 and #35.

@BrknRobot BrknRobot closed this Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants