Extend err_if_nil

[jamessharp]

Found myself wanting to use this just now.

[pdavies]

Hey James, thanks for the contribution!

You’ve run into an issue with this function that was on my radar, but I’m going to try to convince you that this is the wrong way to think about it.

I can see the path that led here - some component considers nil to be a valid value and so it can return {:ok, nil} , but another downstream component cannot handle nil and so we would like the {:ok, nil} to have been transformed into an error by that point. It’s a natural thing to want, and a function called Result.err_if_nil sounds like it would do that! Only, it doesn’t.

Firstly I would say that there are some smells here - we’ve ended up with a lot of function heads, and more importantly, we’ve ended up with a lack of type clarity. Before, we had a function which, to cobble syntax from other languages, looked like

def err_if_nil<T, E>(value: Optional<T>, reason: E) -> Result<T, E>

But now it’s that and

def err_if_nil<T, E, F>(value: Result<Optional<T>, F>, reason: E) -> Result<T, E | F>

Really it’s two different functions in a trenchcoat, choosing different behaviour depending on the input.

A core design goal of this library is to automatically improve type clarity in the vicinity of its use. By typically only acting on certain types (and raising FunctionClauseError otherwise), use of these functions proves something to a reader about the types being handled in the code they are reading. It would be a mistake to allow too much flexibility. That concern still applies for this function even though it's an exception in that it admits any input: this change would have the effect that the input type could no longer be inferred from knowledge of the output type.

The real issue here is that it’s badly named. Most functions in the Result module act on results, and this is an outlier, creating a result from (technically anything but expected to be) a non-result. It’s a converter from Optional<T> -> Result<T> and should probably be called Result.from_optional() instead.

Meanwhile, the behaviour sought by this PR can already be obtained by i_return_a_result() &&& err_if_nil("oh no") (formerly ~>) - I think that’s the right way to do this.

[pdavies]

Renaming err_if_nil to from_optional would free up err_if_nil to be defined as exactly the Result<Optional<T>> -> Result<T> map you're after. It would duplicate &&& from_optional()) but hey, and_then() duplicates &&& Kernel.then(), so that's fair enough. I'd be very happy to merge a change that keeps your new definition of err_if_nil while factoring out the old one into from_optional!