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

Fix Promise#then. #729

Merged
merged 1 commit into from Jun 19, 2018
Merged

Fix Promise#then. #729

merged 1 commit into from Jun 19, 2018

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Jun 8, 2018

The docs say it should work like then(executor: executor).

@pitr-ch
Copy link
Member

pitr-ch commented Jun 8, 2018

Hi, thanks for the fix. However the docs need to be fixed instead at the moment. The change as is would be backward incompatible. (Even though I would also like to use named arguments.)

@pitr-ch pitr-ch added this to the 1.0.6 milestone Jun 8, 2018
@pitr-ch pitr-ch added the bug A bug in the library or documentation. label Jun 8, 2018
@reitermarkus
Copy link
Contributor Author

Maybe we could support both in order to not break compatibility?

@pitr-ch
Copy link
Member

pitr-ch commented Jun 8, 2018

yeah, that's a good plan, however concurrent-ruby has to drop support for 1.9 officially first to be able to do it. So I won't be able to merge this PR until before next minor release.

@reitermarkus
Copy link
Contributor Author

concurrent-ruby has to drop support for 1.9 officially first

Why? There are other methods using named parameters.

@reitermarkus
Copy link
Contributor Author

@pitr-ch, ping.

@pitr-ch
Copy link
Member

pitr-ch commented Jun 18, 2018

Sorry I was away. I think they are using last argument as an options Hash, named arguments should not be used. If you saw named arguments please point me to it.

@reitermarkus
Copy link
Contributor Author

reitermarkus commented Jun 18, 2018

I think they are using last argument as an options Hash

Yes, I just meant that they can be used with named parameters, not that they are implemented with them.

Anyways, I implemented it to support both the current version and the version with an option Hash/named parameter.

Copy link
Member

@pitr-ch pitr-ch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@pitr-ch pitr-ch merged commit 0ee31d0 into ruby-concurrency:master Jun 19, 2018
@reitermarkus reitermarkus deleted the promise-then branch June 19, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants