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
Member

@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

@jodastephen jodastephen commented Dec 21, 2016

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
Member Author

@cjkent 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
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@cjkent cjkent deleted the topic/results-listener branch Dec 21, 2016
@cjkent cjkent removed the Status:InReview label Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.