Skip to content
This repository was archived by the owner on Aug 31, 2022. It is now read-only.

FloRunner refactor #53

Merged
merged 6 commits into from
Mar 8, 2018
Merged

FloRunner refactor #53

merged 6 commits into from
Mar 8, 2018

Conversation

bergman
Copy link
Contributor

@bergman bergman commented Feb 6, 2018

  • make FloRunner easier to test
  • return a useful Result object rather than call System.exit (replaced by FloRunner.runTask(foo).waitAndExit())
  • remove the args to task and the TaskConstructor interface which was used mostly for CLI usage
  • no more apollo
  • no more guava
  • remove json tree logging

@bergman bergman changed the title Flo runner fixes Flo runner changes Feb 6, 2018
@bergman bergman force-pushed the flo-runner-fixes branch 9 times, most recently from 88f3228 to 67f4810 Compare February 8, 2018 16:32
@fabriziodemaria
Copy link
Member

Overall looks good to me.
Regarding the new asynchronous runTask(), we could rename it runTaskAsync(). runTask() could be a blocking version that still returns the resulting value and re-throws any exception from Flo or from within the Tasks.

@bergman bergman force-pushed the flo-runner-fixes branch 4 times, most recently from d27b942 to c9a1a05 Compare February 13, 2018 16:53
@bergman bergman force-pushed the flo-runner-fixes branch from d9b76f2 to 76e42fd Compare March 8, 2018 16:27
@bergman
Copy link
Contributor Author

bergman commented Mar 8, 2018

Overall looks good to me.
Regarding the new asynchronous runTask(), we could rename it runTaskAsync(). runTask() could be a blocking version that still returns the resulting value and re-throws any exception from Flo or from within the Tasks.

I opted for breaking the API. The motivation is that it is still quite early in the project and to me the async should be default.

@bergman bergman changed the title Flo runner changes FloRunner refactor Mar 8, 2018
@rouzwawi
Copy link
Member

rouzwawi commented Mar 8, 2018

LGTM 👍

@bergman bergman force-pushed the flo-runner-fixes branch from b96cb55 to 2d1fc62 Compare March 8, 2018 19:28
@bergman bergman merged commit 726220d into master Mar 8, 2018
@bergman bergman deleted the flo-runner-fixes branch March 8, 2018 19:34
this.future = future;
}

public Future<T> future() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Future (which only allows blocking get()) instead of CompletableFuture or CompletionStage ?

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.

4 participants