assertk icon indicating copy to clipboard operation
assertk copied to clipboard

Make `contains` consistent with the rest of the `contains*` functions

Open adyang opened this issue 4 years ago • 5 comments

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 contains accept vararg (exact implementation of current containsAll)
  • Optionally deprecate containsAll if we do not like the duplication
  • Do the same for doesNotContain and containsNone (In this case keeping containsNone feels more natural)

Benefits of this is that:

  • Users do not have to change from contains to containsAll just 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

adyang avatar May 26 '21 12:05 adyang

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?

evant avatar Jun 09 '21 03:06 evant

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?

adyang avatar Jun 10 '21 10:06 adyang

Yeah I like the idea of documenting them in the README.

evant avatar Jun 10 '21 13:06 evant

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

zoobert avatar Sep 01 '22 18:09 zoobert

Oof that should be fixed

evant avatar Sep 01 '22 22:09 evant