Real-Serious-Games / C-Sharp-Promise

Promises library for C# for management of asynchronous operations.
MIT License
1.2k stars 149 forks source link

Added support for progress reporting. #65

Closed ghost closed 6 years ago

ghost commented 6 years ago

I needed to be able to report progress through promises, so I modified your library to add support for this. ~~Progress reporting is always propagated through the whole chain of promises. You can modify the progress value at any point of the chain if you intend to, for example, average the progress of two promises. For example:~~

IPromise loadAsset(string bundle, string asset)
{
    var promise = new Promise();
    LoadBundle(bundle)
    .Progress(v => promise.ReportProgress(v * 0.5f))
    .Then(b => LoadAssetFromBundle(b, asset))
    .Progress(v => promise.ReportProgress(0.5f + v * 0.5f))
    .Then(() => promise.Resolve())
    .Catch(ex => promise.Reject(ex));
    return promise;
}

loadAsset("bundle", "asset").Progress(v => Debug.Log("Progress: " + v));
RoryDungan commented 6 years ago

This looks like a useful feature, thanks for submitting it!

We require all new code to have comprehensive unit tests and new features to have some documentation, so won't be able to merge this until those have been added.

I've added a CONTRIBUTING.md file with some more details on what we need from pull requests, so hopefully that should be a bit more obvious in future.

ghost commented 6 years ago

Thank you @RoryDungan. I will work on that :) I also need to review the code because I noticed a small issue in the progress propagation. Changes coming soon.

RoryDungan commented 6 years ago

Cool. Just let me know when you're ready for me to have a look over it again 👍

alonsohki commented 6 years ago

@RoryDungan I think this is good to go 😃 Let me know if you find any issues and I will fix it.

RoryDungan commented 6 years ago

Looks good, just need to expand on the tests a bit - see my comments above.

ghost commented 6 years ago

Ready for review ✔️

alonsohki commented 6 years ago

Hi again. Is there anything else required to be able to merge it? It would be a shame if it gets forgotten and becomes non-mergeable :)

RoryDungan commented 6 years ago

Sorry, just got a bit busy with other stuff. It's merged now 😁

alonsohki commented 6 years ago

Thank you :)