Fix filterOrElse to handle null values correctly.

[bwanner]

PR for issue #2932

[nomisRev]

Good catch 🙏 Thank you for raising the issue, and so quickly following up with a PR for a fix! 😍
Congrats on your first contribution as well 🙌

We should keep this issue open since this mistake is likely also made in Option and potentially Ior. Luckily this is not released yet.

[nomisRev]

@bwanner, @dberg, you don't mind the deprecation? If you came across this, I am assuming you were relying on it in your codebase? Hope it didn't cause any issues in production.

[dberg]

Hi @nomisRev, yes we were using it but we've replaced it with a similar impl that has a specialized left value that we use to represent errors. And nope, tests and @bwanner caught the breaking change before any deploys were made.

[bwanner]

@nomisRev - on a related note we defined rightIfNotNull, rightIfNull, leftIfNull in the app itself since we found it useful for the expressiveness / readability.

[nomisRev]

Going to merge the PR, thanks for your contribution and quick action @bwanner 🙏
For the benefit of Arrow, and open an open discussion I wanted to try and gather some feedback from you both if you don't mind 😁

on a related note we defined rightIfNotNull, rightIfNull, leftIfNull in the app itself since we found it useful for the expressiveness / readability.

We're always open to keep them around in Arrow if they're being used often. It's the second time I hear about leftItNotNull, I would personally be more in favour of keeping it aligned with DSL and also recommend the DSL usage ensureNotNull since it comes with all Kotlin necessities like smart-casting. Note that from 1.2.x (2.x.x API, and auto-migrateable) either will not be split in eager vs suspend and comes as a cost-free abstraction.

either { // In the next minor version (with automatic migration) you won't need `suspend/eager` anymore.
  // rightIfNull
  ensure(this == null) { error() }
  
  // rightIfNotNull
  ensureNotNull(this) { error() } // this is smart-casted to non-null after this point.
  
  // leftIfNull
  val b = either.bind()
  ensureNotNull(b) error() } // b is smart-casted to non-null after this point.
}

If find rightIfNull a bit of a weird API, curious what you're using ti for. Treating null as a non error case, and a value is considered an error 🤔

To keep this more in line with the DSL API we could add following APIs:

• either.leftfNull { } -> either.ensureNotNull { }
• a.rightIfNotNull { } -> Either.ensureNotNull(a) { }
• a.rightIfNull { } -> Either.ensure(a == null) { }

At least in this case we could add the same APIs for Option, and stay in line with the same API naming. Albeit I think the DSL is still superior.

[bwanner]

I would personally be more in favour of keeping it aligned with DSL and also recommend the DSL usage ensureNotNull since it comes with all Kotlin necessities like smart-casting. Note that from 1.2.x (2.x.x API, and auto-migrateable) either will not be split in eager vs suspend and comes as a cost-free abstraction.

@nomisRev I agree that just rightIf*Null functions should be sufficient. Sometimes we have one line functions where we don't set up the either context, so we can't directly use ensure:

data class Duck(val name: String)

suspend fun fetchDuck(): Either<String, Duck> = fetchNullableDuck().rightIfNotNull { "Error" }

suspend fun ensureAllowedToFeedDuck(): Either<String, Unit> = fetchFeedDuckDenialReason().rightIfNull { "Not allowed to feed duck :-(" }

I like the idea of adding ensure* functions under Either directly.