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

Implemented .finally(fn), fixing #25 #42

Closed

Conversation

Kaelinator
Copy link
Collaborator

@Kaelinator Kaelinator commented Mar 11, 2018

I am not completely sure if this is implemented correctly. Personally, I have never used .finally, but I read up on its functionality, and have replicated the behavior.
Also, I'm debating whether or not I should merge impl-finally with my master branch...

@Kaelinator Kaelinator changed the title Implemented .finally( ), fixing #25 Implemented .finally(fn), fixing #25 Mar 11, 2018
@Kaelinator
Copy link
Collaborator Author

This is awkward, the commits on #13 are showing up here...

@justsml
Copy link
Member

justsml commented Mar 15, 2018

Thanks @Kaelinator

I've been thinking about Promise.finally() being added to the Promise spec.
I'm not sure I like how it's supposed to work. Kinda makes my techniques harder to reason about. I need more experience with it before I firm up my opinion however. I need some time to think about this some more...

Here's my current thoughts: I'm trying to emphasize that functional-promises is NOT itself a promises replacement.

Adding every feature from Promises into the FP API muddles the message. I should have clarified this on #25

Thank you for the PR, the code looks good - and follows the project style. 👍

Let me know if I missed anything. 😺

@justsml
Copy link
Member

justsml commented Mar 15, 2018

As for the mix of commit history, this happens 100% of the time on 2nd commits. (and sometimes I forget this key step) ;)

You need to pull from the upstream master, merge it into your local master - then create the new branch.

The key is to checkout the upstream master branch, then create the new branch to work on.

If you do it out of order, it's often easier to start a fresh branch and manually copy desired changes back in.

@justsml
Copy link
Member

justsml commented Mar 15, 2018

Ping me on slack if you need some help with the commands...

Let me know if you want to do a pair programming session in the evening this or next week - I need a 2nd set of eyes - been working on a strange bug. 🤔

@justsml justsml mentioned this pull request Mar 15, 2018
@justsml
Copy link
Member

justsml commented Apr 9, 2018

Closing for the moment, putting feature on backlog...

@justsml justsml closed this Apr 9, 2018
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

2 participants