Skip to content

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.

2 participants