It does work. We could add it and mark it deprecated with a note to use empty() instead. ________________________________ From: Daniel Collins <dpcoll...@google.com> Sent: Monday, October 18, 2021 12:35 PM To: dev <dev@beam.apache.org> Subject: Re: [BEAM-13019] Potential breaking change adding matchers to `PAssert.that(pcollection).containsInAnyOrder`
Is there another option to add a `containsInAnyOrder()` overload? I wonder if this would resolve the ambiguity. On Mon, Oct 18, 2021 at 12:32 PM Luke Cwik <lc...@google.com<mailto:lc...@google.com>> wrote: It looks like there isn't any pushback. Let us go with the change as you have suggested. On Wed, Oct 13, 2021 at 1:46 PM Luke Cwik <lc...@google.com<mailto:lc...@google.com>> wrote: I'm for the breaking change but if people have lots of push back I would go with containsInAnyOrder(SerializableMatcher<T> firstMatcher, SerializableMatcher<T>... rest) as my second choice. As a second choice we would be able to swap containsInAnyOrder(SerializableMatcher<T> firstMatcher, SerializableMatcher<T>... rest) with containsInAnyOrder(SerializableMatcher<T>... matchers) and revisit this "breaking" change in the future. Note that there is already an existing method "containsInAnyOrder(Iterable<T> expectedElements)" as well. Should we expose "containsInAnyOrder(Iterable<SerializableMatcher<T>> matchers)"? Another option would be to use a different method name but this isn't what people typically would expect when coming from typical junit/hamcrest/mockito matchers. On Wed, Oct 13, 2021 at 12:22 PM Chris Gray <cg...@ftsinc.com<mailto:cg...@ftsinc.com>> wrote: Hi, I recently noticed that some very useful assertion methods are not available to use in the IterableAssert interface. In particular, I wanted to add containsInAnyOrder(SerializableMatcher... matchers). I did this in https://github.com/apache/beam/pull/15685. However, this causes some tests to not compile because they use containsInAnyOrder() to mean empty(), which causes the compiler to give up because the method is now ambiguous with containsInAnyOrder(T... elements). It seems like there are a few potential ways forward * Commit the breaking change and tell users to use empty() instead of no arguments to containsInAnyOrder. * Change the signature to containsInAnyOrder(SerializableMatcher<T> firstMatcher, SerializableMatcher<T>... rest) * Change the signature to containsInAnyOrder(Collection<SerializableMatcher<T>> matchers). * Do nothing * Something else To me, the first two ways forward are the best. empty() is more descriptive of what is being asserted than containsInAnyOrder(). But if that isn't tolerable, then the second should remove the ambiguity. In any case, please let me know if you have feelings about what should be done. [https://opengraph.githubassets.com/f93657a3199653b0d47e7f5cc35f08f7fa1fe20691c4e17e13dcdd02e2276bb9/apache/beam/pull/15685]<https://github.com/apache/beam/pull/15685> [BEAM-13019] Add `containsInAnyOrder` with matchers to the `IterableAssert` interface by chrismgrayftsinc · Pull Request #15685 · apache/beam<https://github.com/apache/beam/pull/15685> This lets an existing assertion be usable in the PAssert class. Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily: Choose reviewe... github.com<http://github.com>
