willowtreeapps / assertk

assertions for kotlin inspired by assertj
MIT License
757 stars 84 forks source link

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

Closed JakeWharton closed 4 months ago

JakeWharton commented 5 months ago

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":

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

Thoughts?

(Also whoops cmd+enter too soon)

grodin commented 5 months ago

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.

JakeWharton commented 5 months ago

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.

grodin commented 5 months ago

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.)

JakeWharton commented 5 months ago

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).

grodin commented 5 months ago

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

JakeWharton commented 5 months ago

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.

grodin commented 5 months ago

D'oh! Obviously we are!

evant commented 5 months ago

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:

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 commented 5 months ago

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 commented 5 months ago

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

JakeWharton commented 5 months ago

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.

evant commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

grodin commented 4 months ago

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.

cpovirk commented 3 months ago

Hi, I came across this when looking at mentions of Truth. I'll offer a few thoughts, but I'm not sure how relevant they'll end up being.

evant commented 3 months ago

Thanks for the thoughts!

check() is used to implement things like this, but it's available only to users who are defining their own Subject subclass (our equivalent to an AssertJ Assert subclass, which of course you don't need an equivalent to

yep I was aware of that, I left out the distinction for simplicity because the function we are discussing here is used both for implementing your own assertions as well as something that can be used directly.

Truth does have this for collections: "Fuzzy Truth" assertions

Yep, I've been trying to figure out for a while how to fill the same gap, as you point that that approach has pros and cons and and don't exactly love the cons of it. I do think the adjustments here do go in the right direction though I'm not sure yet if it covers all the same use-cases well enough.

I'm not sure I'd use the "hasMessageThat" style if we were doing it again.

The current proposal is to use the style: havingMessage() (see https://github.com/willowtreeapps/assertk/pull/537/files). I'm not completely sold on this yet as opposed to the current shorter message() syntax, but does bring some consistency here and https://github.com/willowtreeapps/assertk/issues/522 correctly points out that it can't be used for all functions.