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

sortedWith (and related) matchers. #383

Merged
merged 6 commits into from
Jul 18, 2018
Merged

sortedWith (and related) matchers. #383

merged 6 commits into from
Jul 18, 2018

Conversation

outis
Copy link
Contributor

@outis outis commented Jul 17, 2018

Here are some matchers like sorted() (and those based on it), but that take custom orderings.

The commit also includes a new test subproject to test assertions.

@sksamuel
Copy link
Member

Awesome! but can I ask for a couple of things before merging.

  1. That the matchers are placed in io.kotlintest.matchers.collections/matchers.kt
  2. That we add extension function versions of them too to keep it consistent.

@outis
Copy link
Contributor Author

outis commented Jul 17, 2018

  1. I've moved the matchers from CollectionMatchers.kt to collections/matchers.kt. Is that your plan for the sorted() and beSorted() matchers in CollectionMatchers.kt?
  2. I think the extension functions you want (e.g. List<T>.shouldBeSortedWith(...)) are already present in collections/matchers.kt. Were you referring to something else?

The matchers were suffixed with "With" to match the convention in collections (Iterable<T>.sortedWith(...)), but "By" may be more readable in use (e.g. countdown.sortedBy(desc)). Which do you think is the better naming scheme?

I also tweaked the tests, but that should be an inconsequential change (as far as this feature is concerned).

@sksamuel
Copy link
Member

  1. Only the new ones, the old ones can stay where they are for now, to avoid people changing imports for existing ones. I may migrate them slowly across as we bump versions.
  2. Yes sorry, I missed that first time around.
  3. As for naming, I think either sounds alright, so whatever you prefer.

@sksamuel sksamuel merged commit 297470e into kotest:master Jul 18, 2018
@outis
Copy link
Contributor Author

outis commented Jul 19, 2018

If it were just one of the two suffixes, "by" would have been my choice (it's also slightly more common). However, since "by" is used for methods that take selectors, it's probably least surprising to stick with the convention established by library classes. Speaking of "by", would it be worth it to add sort matchers taking selectors? They're not necessary, as the matchers with comparators are more general, but might be of use.

@sksamuel
Copy link
Member

I think it's fine as it is, but if you think otherwise I'd be happy to merge another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants