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

Create a public CalculationListener returning a future #1454

Merged
merged 1 commit into from Dec 21, 2016

Conversation

cjkent
Copy link
Contributor

@cjkent cjkent commented Dec 21, 2016

This PR renames AggregatingListener and moves it to the top level so it can be used by external code.

Having a CalculationListener returning a CompletableFuture is essential for writing fully asynchronous code with Strata. It seems a shame that users will have to implement it themselves when we already have one in Strata.

@jodastephen
Copy link
Member

Approved, as there is nothing wrong with this. However, the new public class does seem a little isolated. Another problem is that the user has is they need to pass in the columns to the ResultsListener (and typically other Strata classes have factory methods). Not sure there is a way to setup the columns automatically, but it would be nice. Maybe a calculationStarted(columns) method on CalculationListener ?

@cjkent
Copy link
Contributor Author

cjkent commented Dec 21, 2016

I agree that it's not particularly elegant to pass the columns to the listener and the calculation runner. I like your suggestion. I'll merge this PR and raise a follow up to add calculationStarted()

@cjkent cjkent merged commit 6e8aabd into master Dec 21, 2016
@cjkent cjkent deleted the topic/results-listener branch December 21, 2016 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants