willowtreeapps / assertk

assertions for kotlin inspired by assertj
MIT License
760 stars 85 forks source link

Suggestion: deprecated `apply` member on `Assert` #455

Closed JakeWharton closed 1 year ago

JakeWharton commented 1 year ago

In migrating from Truth I had a lot of assertions in the form

assertThat(subject).apply {
  contains(foo)
  contains(bar)
}

I think migrated them all to all, but honestly I have no idea. Searching for this is also quite hard.

Realistically there should be zero valid usages of apply on an Assert. At least not in a way where the receiver type actually is Assert. Could you call apply on an Any containing an Assert instance? Sure, although wtf are you even doing there?

Anyway, I would consider doing

class Assert<T> {
  @Deprecated(
    "Use Assert.all for multiple assertions and to see all failures at once",
    ReplaceWith("this.all(body)", "assertk.all"),
    DeprecationLevel.ERROR,
  )
  fun apply(body: Assert<T> -> Unit): Assert<T> = throw AssertionError() // Never invoked.
}

Maybe ERROR is taking it too far, but also... I don't think it is. At least a warning would be nice.

If you don't want to do either perhaps wait until https://youtrack.jetbrains.com/issue/KT-54106/Provide-API-for-perpetual-soft-deprecation-and-endorsing-uses-of-more-appropriate-API where it can be done without even a build warning.

JakeWharton commented 1 year ago

Searching for this is also quite hard.

It's actually easy within a single compilation unit as you just define the extension on Assert<T>, but if you have a large project with say 1000 modules then it becomes much harder.

evant commented 1 year ago

To answer why you'd want to use apply over all, it's a choice on if you want to opt into soft assertions or not. Apply will fail at the first failure, all will run all of them.

Now is that an actual useful distinction in practice? Not really sure, I personally have never needed apply. But I could see it being useful in not breaking tests that were written with a fail-fast assumption in mind?

Thinking out loud, I wonder if this would make more sense as a lint (say with detekt), I can actually imagine a few use-cases for some, assertThat without an assertion would be another example.

JakeWharton commented 1 year ago

Wouldn't you use chaining rather than apply in that case?

evant commented 1 year ago

https://github.com/willowtreeapps/assertk/discussions/346

JakeWharton commented 1 year ago

Ah, right. Right.

Well I still remain skeptical that you'd ever want to use it over all, but I suppose there's no real harm in either the failure or success case.

JakeWharton commented 1 year ago

We don't use detekt generally but the repo has ktlint where I think I could write a rule.

Since it doesn't really affect anything I'm just going to say it's not really worth doing anything.