assertk icon indicating copy to clipboard operation
assertk copied to clipboard

Consider renaming/unifying `prop`/`extracting`/`suspendCall`

Open JakeWharton opened this issue 1 year ago • 7 comments

prop isn't short for property–at least not in the Kotlin-sense. Its KFunction and lambda-accepting overloads let it operate on more than just Kotlin properties. It may be short for "property" in the general sense, but that's then confusing against the Kotlin concept and abbreviations are generally frowned upon in API signatures.

extracting is prop projected over a collection.

suspendCall is the lambda-accepting prop but suspendable.

These are all the same thing, and it would be cool if they had a unified naming convention.

I don't have any perfect naming proposals yet but I've been sitting on filing this issue for months now so I'm firing it off in the hopes others can help.

I came up with two ideas:

Center around the verb "have":

  • Assert that some user has (a) name (that) is equal to "Alice":

    assertThat(someUser).has(User::name).isEqualTo("Alice")
    
    assertThat(someUser).has("name", { it.name() }).isEqualTo("Alice")
    
  • Assert that all users each has (a) name (that) is equal to "Alice", "Bob", "Eve":

    assertThat(allUsers)
        .eachHas(User::name)
        .isEqualTo(listOf("Alice", "Bob", "Eve"))
    

    Is this eachHave? Tempted to bend grammar even if it is for consistency.

  • Assert that some user has (a) DB name (that) is equal to "Alice":

    assertThat(someUser).has("name", { it.dbName() }).isEqualTo("Alice")
    

    Can we pull this off, where the signature matches? Can the main one be inline with some API changes maybe?

Alternatively, lean into map whose semantics are hopefully known from its available on Iterables, Flows, Rx, other languages, etc.

  • Assert that some user map(ped to a) name (that) is equal to "Alice":

    assertThat(someUser).map(User::name).isEqualTo("Alice")
    
    assertThat(someUser).map("name", { it.name() }).isEqualTo("Alice")
    
  • Assert that all users map(ped to their) name (that) is equal to "Alice", "Bob", "Eve":

    assertThat(allUsers)
        .map(User::name)
        .isEqualTo(listOf("Alice", "Bob", "Eve"))
    

    Is this eachHave? Tempted to bend grammar even if it is for consistency.

  • Assert that some user map(ped to a) DB name (that) is equal to "Alice":

    assertThat(someUser).map("name", { it.dbName() }).isEqualTo("Alice")
    

Thoughts?

(Also whoops cmd+enter too soon)

JakeWharton avatar Mar 19 '24 02:03 JakeWharton

Just a comment on the proposal for using "have": a possible alternative to has could be having. The idea is that the really natural word for these is probably "with" but that is already well-used kotlin syntax so would be a very poor choice. "Having" is kind of a synonym for "with", so might read more naturally.

It would look like this:

assertThat(someUser).having(User::name).isEqualTo("Alice")

assertThat(someUser).having("name", { it.name() }).isEqualTo("Alice")

Then eachHas/eachHave would be eachHaving.

I'll try to open a PR so more code can be written against it to see what it feels like.

grodin avatar Mar 20 '24 17:03 grodin

Yeah I chose "has" because I thought Truth used it pervasively, but it seems like they use it in quite limited fashion. The ones I use constantly are ThrowableSubject.hasMessageThat and ThrowableSubject.hasCauseThat when asserting on failures. I really thought they had more, but it doesn't seem like it.

Their use of the That-suffix is interesting. I parenthesized it out of my natural language sentence equiavlents, but I suppose it could have been included. It would be nice to choose something that didn't require both a prefix and a suffix, though, and maybe "having" gives us that.

JakeWharton avatar Mar 20 '24 17:03 JakeWharton

I've just realised, it's not going to be possible to unify prop and suspendCall since prop is an instance method and suspendCall is an extension method, so giving both the same name will mean the suspend fun is shadowed.

If only you could have a suspend fun overriding a non-suspend fun (I understand why that's not possible, I'm just musing.)

grodin avatar Mar 20 '24 18:03 grodin

prop is also an extension. I think its implementation APIs (transform and appendName) are both public so it probably could just be made inline so that it works in both contexts (like assertFailure does).

JakeWharton avatar Mar 20 '24 19:03 JakeWharton

Would that be an ABI break? Might be worth doing it separately to this if it is.

grodin avatar Mar 21 '24 18:03 grodin

We're already renaming the functions and breaking everyone. The deprecation replace with for both prop and suspendCall can both target the new, single function.

JakeWharton avatar Mar 21 '24 18:03 JakeWharton

D'oh! Obviously we are!

grodin avatar Mar 21 '24 18:03 grodin

prop isn't short for property–at least not in the Kotlin-sense. Its KFunction and lambda-accepting overloads let it operate on more than just Kotlin properties.

It originally was but became more general shortly after, I agree with the sentiment of this issue.

Yeah I chose "has" because I thought Truth used it pervasively, but it seems like they use it in quite limited fashion. The ones I use constantly are ThrowableSubject.hasMessageThat and ThrowableSubject.hasCauseThat when asserting on failures. I really thought they had more, but it doesn't seem like it.

that's an interesting connection. We do actually have a good number of 'has*' methods which may make this usage more natural.

For reference:

  • assertj calls both the object and collection methods 'extracting'
  • truth calls the object method 'check' and doesn't have a collection equivalent (lmk if this is wrong)

I think its implementation APIs (transform and appendName) are both public so it probably could just be made inline so that it works in both contexts (like assertFailure does).

there was a reason this wasn't done, but I forget why off the top of my head

evant avatar Mar 25 '24 16:03 evant

Another note: kotlin itself doesn't unify on a name for their similar concept (let vs map), so while I do strongly agree that prop & suspendCall should be renamed I feel much less strongly that extracting should.

evant avatar Mar 25 '24 16:03 evant

Linking https://github.com/willowtreeapps/assertk/issues/522#issuecomment-2018493890 as the same argument might be relevant here, a point against 'has' is that it may sound like an assertion itself instead of a middle step, ex someone might do:

assertThat(someUser).has(User::name)

not realizing it will never fail

edit: This makes me like the 'having' suggestion above a bit more, I feel like it has less of that connotation

evant avatar Mar 25 '24 22:03 evant

Indeed Kotlin doesn't use the same name, although I also don't think that's a strong design decision on their part. map is basically forced on them, and let seems more designed within the let/run/with/apply set.

One important difference is that let and map operate directly on the subject whereas the functions here operate on the subject through an Assert wrapper. I think you could make an argument for having the ones here use the same root name(verb?) for the operation, but use something added like an "each" prefix or suffix when operating within an iterable.

JakeWharton avatar Mar 25 '24 22:03 JakeWharton

Also there's a couple of other ways that extracting is different from prop today. It does not take a name to prepend to the failure message, and it has overloads that allow you to extract 2 or 3 values at a time. (the second on is helpful for when you want to be more specific on what you assert on but still use the contains* methods.)

ex:

assertThat(users).extracting(User::name, User::age).containsExactly(
    "Alice" to 22,
    "Bob" to  19
)

evant avatar Mar 25 '24 23:03 evant

One thing worth exploring would be to add in addition to/replace extracting with a version of eachHaving that more directly mirrors having ex:

assertThat(allUsers).eachHaving(User::name).isEqualTo(listOf("Alice", "Bob", "Eve"))

assertThat(allUsers).eachHaving("name") { it.name }.isEqualTo(listOf("Alice", "Bob", "Eve"))

evant avatar Apr 01 '24 16:04 evant

Since I haven't seen any other suggestions given and can't think of anything better, I think we should go with the having pattern.

evant avatar Apr 17 '24 22:04 evant

One thing worth exploring would be to add in addition to/replace extracting with a version of eachHaving that more directly mirrors having ex:

assertThat(allUsers).eachHaving(User::name).isEqualTo(listOf("Alice", "Bob", "Eve"))

assertThat(allUsers).eachHaving("name") { it.name }.isEqualTo(listOf("Alice", "Bob", "Eve"))

I hadn't read this properly when you first posted it. I think this is a really good idea. When #524 is merged, I'll get another draft PR opened.

grodin avatar Apr 18 '24 21:04 grodin