robstoll/atrium

deprecate ...Nullable overloads

robstoll opened this issue · 4 comments

robstoll commented

The original idea was to: deprecate ...Nullable overloads if subject is non-nullable
I changed the idea to deprecate it always.

*Platform* (JVM and/or JS): all

Code related feature

For instance:

assert(listOf(1, 2)).containsNullableValue(null)
robstoll commented

I am not entirely sure if this is a good idea as it pollutes the API. Maybe we should rethink the API instead so that we don't have to distinguish between nullable and non-nullable?

robstoll commented

Rethinking the API has a lot to do with #56 and the following questions:

  • should we show to the user via API that he deals with nullable types?
  • do we want to prevent that a user can compare a non-nullable type against null?

Let's discuss this based on assert(mapOf("a" to 1)).containsNullable(null). Currently that's possible. If we are going to take out away from Assert, then we could remove containsNulllable and only provide contains with V: Any?.
Pro:

  • less functions
  • IntelliJ suggest to pass Pair<String, Int> to contains

Contra:

  • one can also pass "a" to null because V in Pair is covariant, we could introduce our own Pair to deal with this or even better -> we need user site variance restriction to deal with this (https://youtrack.jetbrains.com/issue/KT-24799) or lower bounds
    => yet, the test will always fail, so as long as the user runs a written test (which is almost always the case) it's not too much of a bummer
  • the user does no longer see if the value type is actually nullable (questionable if one really cares, maybe this one is a bit too educational/opinionated from my part -> seeing nullable implies that one should write a case for null as well -- or rethink the API)

Alternative, we add deprecated overloads for containsNullable with <V: Any>.
Pro:

  • we still have distinction between nullable and non-nullable types
  • user gets hint if she tries to use containsNullable on a non-nullable value type

Contra:

robstoll commented

Another thing to consider , I got the feedback from several people that reading the following:

assert(x).contains.inAnyOrder.atLeast(1).nullableValue(1)

is confusing because 1 is not nullable. Others are even confused to read (at least at first):

assert(x).contains.inAnyOrder.atLeast(1).nullableValue(a)
robstoll commented

I have made up my mind after getting several feedback. I'll reduce complexity in the future by no longer distinguishing between AssertionPlant and AssertionPlantNullable. In the same go I will deprecated functions which contain nullable in its identifier. Moreover, shortcut functions for nullable properties will not get an overload with an assertion creator (otherwise we will ran into overload ambiguity issues).
I'll adapt the new functions added in 0.8.0-alpha so that 0.8.0-beta is already prepared for the future.

That said, it would be possible to pass null to functions like toBe in the future even if we have a non-nullable subject -- which means this will always fail and does not make sense, but that's fine.
I'll probably also add an Assert<T> with an invariant T in the future so that we can circumvent this problem.