ApplyOnFailure extension method for Result<Unit, TFailure>

[RyanMarcotte]

No description provided.

[RyanMarcotte]

I debated whether the ApplyOnFailure method should be for all Result<TSuccess, TFailure> types or just Result<Unit, TFailure>... went with the latter because we marked Apply(Action<TSuccess>) as obsolete and steer developers toward using Apply(Action<TSuccess>, Action<TFailure>).

I'm open to being convinced otherwise.

[JohannesMoersch]

The version of Apply which does not require handling for failure is currently marked as obsolete. The idea was that when terminating a chain of functions you should always handle both cases. If we put this in, we'd also need to un-obsolete those methods. I'm a bit on the fence.

[RyanMarcotte]

For a bit of background, ApplyOnFailure is an extension method I ended up writing for a couple client projects that am now just proposing for the library.

The version of Apply which does not require handling for failure is currently marked as obsolete. The idea was that when terminating a chain of functions you should always handle both cases. If we put this in, we'd also need to un-obsolete those methods.

I disagree that the other methods need to be un-obsoleted for the generic Result<TSuccess, TFailure>: I would keep them marked Obsolete. In the case of Result<Unit, TFailure> specifically, I've been writing result.Apply(_ => { /* DO NOTHING */ }, error => HandleFailure(error)) more often than not as Result<Unit, TFailure> is a specialized case IMO.

I can see how it might be (initially) confusing to library consumers though.

[deimors]

If I recall, the reasons for explicitly handling both cases for Apply were:

• For clarity, to make it obvious that there are two cases which could occur and both should be considered
• More specifically, to make it more difficult to forget about the possibility of error cases ("hill of failure", as opposed to "pit of success")

However, I think it's the specificity of ApplyOnFailure that could make it exempt from those reasons. When it's in the name, it's much harder to miss that you're only handling one case.

Where I sit fence-wise is my uncertainty around just defining it for <Unit, TFailure>, I don't really see what this specific (albeit "special", and possibly common) success type has to do with wanting to do something in the case of failure.

[RyanMarcotte]

If I recall, the reasons for explicitly handling both cases for Apply were:

• For clarity, to make it obvious that there are two cases which could occur and both should be considered
• More specifically, to make it more difficult to forget about the possibility of error cases ("hill of failure", as opposed to "pit of success")

I agree with this, which is why I constrained ApplyOnFailure to only work for Result<Unit, ...> (since I use Unit return type to mean "it happened, there is no associated data" and in many cases there is no extra processing). If the success type is anything but Unit then we should continue to force users of the library to handle those cases explicitly.

Example usage:

The Result returned from _mediator.Send(new UploadAllProductSkusToSquareRequestParameters(new UserReferenceId(UserContext.CurrentUserId)), cancellationToken).AsResult() is Result<Unit, SquareApiErrorResponse>. We have similar failure-handling code in other legacy parts of the application that have page redirects to other MVC-controller endpoints.

Could use Do(_ => ...).Do(_ => ...).DoOnFailure(error => ...) for the end of the chain, but ending on Do or DoOnFailure looks awkward to me.

[deimors]

It's an interesting example you posted, I might suggest a refactor:

_mediator.Send(...)
  .AsResult()
  .Apply(
    _ => {
      AddFlashMessage(...);
      LogUserEvent(...);
    },
    error => MakeError(error)
  );

Not that I believe you're referring in general to cases that always involve a .Do(...).Apply(_ => {}, error => ...), but in this specific case, why not just make use of both sides of the Apply?

Anyways, I lean towards adding ApplyOnFailure but as the more general Result<TSuccess, TFailure> extension. I just picture an annoyed me in the future having to .Select(_ => Unit.Value).ApplyOnFailure(...) something and grumbling about it. If I'm already going to the trouble of explicitly saying "I'm only doing this on a failure", why should I also have to explicitly massage the success type into "I don't care" (ie: Unit)?

Or maybe there is value in being that explicit? If so, I'd probably introduce an extension method .IgnoreSuccess() that just does a .Select(_ => Unit.Value)...

[JohannesMoersch]

Just coming back to this. Sorry, slipped my mind. If we add ApplyOnFailure, should there also be an ApplyOnSuccess?

[deimors]

Yea, I think the same argument about the naming being explicit also applies to ApplyOnSuccess, and it would make the library more comprehensive in this regard.

[RyanMarcotte]

Or maybe there is value in being that explicit? If so, I'd probably introduce an extension method .IgnoreSuccess() that just does a .Select(_ => Unit.Value)...

I'm on board with that

Also on board with adding ApplyOnSuccess alongside ApplyOnFailure