Make `contains` consistent with the rest of the `contains*` functions
Currently, Assert<Iterable<*>>.contains only accepts a single element and for multiple elements we have Assert<Iterable<*>>.containsAll(vararg elements: Any?).
containsAll is also able to accept a single element:
assertThat(listOf("one", "two")).containsAll("one")
It seems like in this case, contains feels unnecessary and its behaviour of accepting only a single element is inconsistent with the other contains* functions which accept both single and multiple elements.
Would it help in consistency and maintenance if we do something like this:
- Make
containsacceptvararg(exact implementation of currentcontainsAll) - Optionally deprecate
containsAllif we do not like the duplication - Do the same for
doesNotContainandcontainsNone(In this case keepingcontainsNonefeels more natural)
Benefits of this is that:
- Users do not have to change from
containstocontainsAlljust to use more elements - Consistent interface with other
contains*methods - Easier to maintain since we will not have more combinations of single vs multiple element functions
The thought process here is that with a single element it's obvious what contains does. But with multiple it is a bit unclear, hence the names: containsAll(), containsExactly(), containsOnly(), containsSubList().
Is it obvious that contains would have the behavior of containsAll above? Ex, if you saw:
assertThat(listOf("one", "two")).contains("two", "one")
would you be surprised that it passed?
After reading the explanation, I think I understand your perspective- because contains usually have a single element signature on Collections, users might expect the same applies for assertions. For multiple elements, a different name with contains* will make it clearer it is for multiple elements. Is this right?
Coming from AssertJ, I would not be surprised that it passed, though I think other users might find otherwise. Regardless, I think one of struggles is understanding the different contains*, maybe we can document these different methods in the README? Kind of like in: https://assertj.github.io/doc/#assertj-core-group-contains
Should I create a new issue/ pull request?
Yeah I like the idea of documenting them in the README.
Some of the examples the code seem to be in conflict with the discussion here. The example in the code link below seems to advocate for using contains with more than one item. To be clear, the examples in the code below do not work with the current implementation of contains and should probably be changed. It threw me for a loop and led me to this discussion for clarification.
https://github.com/willowtreeapps/assertk/blob/main/assertk/src/commonMain/kotlin/assertk/assertions/iterable.kt#L146
Oof that should be fixed