willowtreeapps / assertk

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

Consider unifying first-party, `prop`-based helper function names (like `jClass`, `toStringFun`) #522

Open JakeWharton opened 5 months ago

JakeWharton commented 5 months ago

The current naming is mostly just the property name as the function name. There's kClass(), collection size()s, exception message().

This strategy doesn't work for toString() or hashCode(). They get "Fun" suffixes as toStringFun() and hashCodeFun(), respectively.

This ties in somewhat with #521, and it actually leans me towards that issue choosing "have" so that these could be "has"-prefixed.

assertThat(foo).hasKClass().isEqualTo(Foo::class)
assertThat(array).hasSize().isEqualTo(3)
assertThat(user).hasToString().isEqualTo("User[name=Alice]")
grodin commented 5 months ago

Once there's an agreement on how to resolve #521, happy to handle this

evant commented 5 months ago

There is a subtle distinction there, the 'has*' methods do an assertion themselves while the non-has methods can't fail on their own. One thing I'd want to avoid is someone doing assertThat(array).hasSize() by itself not realizing that it will never fail.

grodin commented 5 months ago

Perhaps "having" could help here, as it seems less like an assertion:

assertThat(list).havingSize()

feels less like a complete sentence.

JakeWharton commented 5 months ago

I also think having pairs well with extracting, should we opt not to change it to something like eachHaving.

grodin commented 5 months ago

I also think having pairs well with extracting, should we opt not to change it to something like eachHaving.

Perhaps that's a useful general principle for this kind of thing; a method named with a verb which isn't an assertion should use the continuous ("ing") form of the verb.

That's almost certainly a premature abstraction, and wrong in the details, but I thought it was worth noting down.

evant commented 3 months ago

Hm, looking at the draft mr I'm not sure I'm convinced that adding a having prefix to everything is preferable to the status-quo. There are far more methods that the current pattern works for than it doesn't. I think it would still be reasonable to rename toStringFun and hashCodeFun to havingToString and havingHashCode respectively, but thinking it may be better to keep the rest the same. Thoughts?

JakeWharton commented 3 months ago

What are your primary examples of occurrences you don't like?

evant commented 3 months ago

From a wider context we are introducing api churn. Which in-itself isn't a bad thing but does mean that there should be a good reason for it.

Let's take message() for example. We'd go from:

assertThat(exception)
    .message()
    .isNotNull()
    .matches(Regex(...))

to

assertThat(exception)
    .havingMessage()
    .isNotNull()
    .matches(Regex(...))

The first one is shorter and reads slightly better. Like it's not a huge difference but if we are going to change things it should be a more obvious win no?

evant commented 3 months ago

note: this was partially spurred on by https://github.com/willowtreeapps/assertk/issues/521#issuecomment-2098804966, which also points out than androidx has started using a similar pattern in their [truth extensions](https://developer.android.com/reference/androidx/test/ext/truth/app/NotificationSubject#contentIntent())

JakeWharton commented 3 months ago

I actually think the havingMessage one reads better, because every function call is in the form of verb-noun, either by itself or in conjunction with the supplied argument.

It produces "assert (that) exception having message is not null matches regex".

It also means that you can type ".having" to see all built-in transformations as well as complete to the lambda variation to do custom transforms.

grodin commented 3 months ago

FWIW, I agree with Jake. I think the consistency of all helper functions starting with "having" helps distinguish them.