Skip to content
This repository has been archived by the owner on Jan 15, 2022. It is now read-only.

Feature/futures #4

Merged
merged 2 commits into from Aug 18, 2013
Merged

Feature/futures #4

merged 2 commits into from Aug 18, 2013

Conversation

daggerrz
Copy link
Contributor

Adds support for Twitter and Scala futures by adding new Intent types: FuturePlan.Intent and TwitterFuturePlan.Intent.

It also makes future pool for regular cycle intents configurable so that blocking operations can be performed.

@softprops
Copy link

have had a lot of time to look at this closely but at a high level it looks good to me.

@chrislewis
Copy link
Contributor

Thanks Dag!

UnfilteredService line 20, the cycle intent constructor takes a future pool argument but doesn't propagate that to the cycle call. I assume that's an oversight but it raises another question: what is gained by having the 3 apply methods merely proxy to the explicitly named methods (cycle, twitterFuture, scalaFuture)? I'm inclined to remove them but I want to make sure I'm not missing anything.

@daggerrz
Copy link
Contributor Author

Yes, that's an oversight from when I added the overloaded methods, I'll get it updated tomorrow.

As for the overloading itself, I agree that it's not very pretty. The primary reason for adding them was to stay consistent while not breaking backwards compatibility. I'd probably prefer just removing all of them.

@chrislewis
Copy link
Contributor

Ok no worries - I can tweak it. As for backwards compatibility, I assume you mean with extensions you've made internally (that use twitterFuture, etc)? If that's the case I'm happy to leave them there for a version or two if you have dependencies.

@daggerrz
Copy link
Contributor Author

No problem breaking things for us - I just noticed you deprecated a method in your code, so I was trying not to break anything else.

@chrislewis chrislewis merged commit 9e2425e into novus:master Aug 18, 2013
@chrislewis
Copy link
Contributor

Merged. Have a look at HEAD but there shouldn't be anything objectionable in there. Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants