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

Expand Container dependsOn parameters to accept Iterable #2259

Merged
merged 5 commits into from Apr 5, 2020

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented Jan 18, 2020

Builds upon #2221 so that any Iterable is allowed as a dependsOn argument.
Suggested by @bsideup

@@ -26,9 +26,6 @@
import lombok.Setter;
import lombok.SneakyThrows;
import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that there seem to be a lot of import movements. I'm not sure why this is suddenly happening, but I'm confident that the .editorconfig rules we have set up are being respected:

[*.java]
indent_style = space
indent_size = 4
# Never use star imports
ij_java_names_count_to_use_import_on_demand = 99
ij_java_class_count_to_use_import_on_demand = 99
ij_java_layout_static_imports_separately = true

Maybe recent edits to these files have not been picking up the rules?

kiview
kiview previously approved these changes Feb 28, 2020
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

Great API improvement.

*/
public SELF dependsOn(List<Startable> startables) {
dependencies.addAll(startables);
public SELF dependsOn(Iterable<? extends Startable> startables) {
Copy link
Member

Choose a reason for hiding this comment

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

this will break binary compatibility

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushing a commit now to reinstate dependsOn(List)

@rnorth rnorth requested review from kiview and bsideup March 1, 2020 16:37
@rnorth rnorth dismissed kiview’s stale review March 5, 2020 20:54

@bsideup had concerns, hopefully now resolved

Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

👍

@bsideup bsideup added this to the next milestone Mar 13, 2020
@@ -27,8 +30,8 @@ public Thread newThread(Runnable r) {
/**
* @see #deepStart(Stream)
*/
public CompletableFuture<Void> deepStart(Collection<? extends Startable> startables) {
return deepStart(startables.stream());
public CompletableFuture<Void> deepStart(Iterable<? extends Startable> startables) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, japicmp has flagged this!

Just as we did in GenericContainer, I'll reinstate the previous method signature for binary compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reinstated.

@rnorth rnorth changed the title Expand dependsOn parameters to accept Iterable Expand Container dependsOn parameters to accept Iterable Apr 5, 2020
@rnorth rnorth merged commit a09f4d5 into master Apr 5, 2020
@rnorth rnorth deleted the depends-on-iterable branch April 5, 2020 17:59
@rnorth
Copy link
Member Author

rnorth commented Apr 13, 2020

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020
…tainers#2259)

* Expand `dependsOn` parameters to accept `Iterable`
* Reinstate a `dependsOn(List)` method, for binary backwards compatibility
* Reinstate `deepStart(Collection<? extends Startable>)` method
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

3 participants