Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[e2e] Use SettableFutures instead of CompletableFuture #2854

Merged
merged 2 commits into from
Jul 1, 2020

Conversation

jiahaog
Copy link
Member

@jiahaog jiahaog commented Jun 30, 2020

Description

This unblocks the import of the plugin internally.

Related Issues

b/160208320

Breaking Change

Breaking change if users depend on E2EPlugin.testResults as an instance of CompletableFuture.

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

This works better for internal use cases.
@jiahaog jiahaog force-pushed the dont-use-completable-future branch from 5d17791 to 39fbe2c Compare June 30, 2020 08:07
@jiahaog jiahaog requested a review from dnfield June 30, 2020 13:23
@jiahaog jiahaog marked this pull request as ready for review June 30, 2020 13:23
@jiahaog jiahaog requested a review from digiter as a code owner June 30, 2020 13:23
public static CompletableFuture<Map<String, String>> testResults = new CompletableFuture<>();
private static final SettableFuture<Map<String, String>> testResultsSettable =
SettableFuture.create();
public static final ListenableFuture<Map<String, String>> testResults = testResultsSettable;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make this be a Future instead of needing to publicly export its type?

I'm a little concerned that at some point we'll have a reason to avoid using Guava for this, and we'll have to break people again. I'm also not clear on what benefits consumers get from having this be a ListenableFuture rather than just a Future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, done

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

@jiahaog
Copy link
Member Author

jiahaog commented Jul 1, 2020

Thanks!

@jiahaog jiahaog merged commit 71cc865 into flutter:master Jul 1, 2020
@jiahaog jiahaog deleted the dont-use-completable-future branch July 1, 2020 00:02
@jiahaog
Copy link
Member Author

jiahaog commented Jul 1, 2020

Sorry, I've never done this before, should I follow https://github.com/flutter/plugins/blob/master/CONTRIBUTING.md#the-release-process to release it onto pub?

@dnfield
Copy link
Contributor

dnfield commented Jul 1, 2020

Yes. If you need permissions @kf6gpe can help

jiahaog added a commit that referenced this pull request Jul 1, 2020
Missed this earlier in #2854
dnfield pushed a commit that referenced this pull request Jul 1, 2020
agent3bood pushed a commit to agent3bood/flutter-plugins that referenced this pull request Jul 10, 2020
Expose a `Future` for `testResults` instead.

This works better for internal use cases.
agent3bood pushed a commit to agent3bood/flutter-plugins that referenced this pull request Jul 10, 2020
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
Expose a `Future` for `testResults` instead.

This works better for internal use cases.
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Expose a `Future` for `testResults` instead.

This works better for internal use cases.
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants