Consider adding support for global error handling

[brendanlensink]

This could be accomplished through delegate methods or some some of combine publisher.

[brendanlensink]

This is a big enough change, I think it might be worth doing a little bit of a proposal before diving right in. So, here's my proposal for how to go about this:

1. Change the Netable.request function call from using a single onCompletion: @escaping (Result<T.FinalResource, NetableError>) -> Void to: onSuccess: Result<T.FinalResource>), onFailure: ((NetableError) -> Void)?
2. Add an optional requestErrorDelegate to Netable with one callback function: requestDidFail(request: Request, error: NetableError).
3. Add an optional errorPublisher: Publisher<Request, NetableError> to Netable.
4. Change the request error handling to do the first of these that's true: a. Check if onError was defined, and if so call that b. Check if errorPublisher or requestErrorDelegate are defined, and if so call those c. Do nothing (do we want to log some kind of warning here that a request failed with no error handling, or assume it was intentional?)

@nbrooke @jenncoop does this make sense to you? Anything you'd do differently?

[nbrooke]

1 and 4 make me a little more nervous (2 & 3 seem conceptually pretty good), for two reasons.

One: Switching from results to callbacks would be a pretty big API break for existing uses, so at the very least, I think we would want add the new two callback form of request rather than removing the result version (which complicates the proposed semantic of point 4 quite a but)

Two: I'm not convinced that the semantics of the local error handling overriding the global error handling, or needing to give up on getting error callbacks locally to have global error handling are the right ones. I can imagine some pretty simple cases where you would want both some global error handling (log the error, handle global state changes like logging out on auth errors) AND some local error handling (show or hide UI on this specific screen when the request completes, whether it fails or not) on the same request.

Consider how we'd switch something like this:

hud.showSpinner()
api.request(Foo()) { result in
    hud.hideSpinner()
    switch result {
       case .success(let foo): doneFoo(foo)
       case .failure(let error): globalFailure(error)
    }
}

to that API for example, and wanting to move globalFailure into global error handling (assume all the requests are calling it). We can't hide the spinner locally when this request fails (because we get no local callback when it fails), adding the spinner hide to globalFailure might not make sense, and even if you just fall back to doing everything by hand and ignoring global error handling for this specific call, you now need to put hideSpinner in both the success and failure callbacks, which is not great.

So I think probably the default should be to call both local and global error handling (which then doesn't really require a new non-result version of the request call)

If a specific request DOES need to disable global error handling in favour of only local error handling, it'd problaby be better to have a way to specifically make that happen. (maybe an "allowGlobalErrorHandling" flag, either as a property of the Request object or a parameter to request.

[brendanlensink]

@nbrooke That all makes a lot of sense. If we're going to keep around completion, do you think it makes sense to add separate success and failure calls, or does it make more sense to just encourage users to always use completion and use an if case statement if they're only interested in the success or failure options?

Adding the 2 more callbacks and having them all be optional may be a little clearer, but it does complicate the internal code (you need to make sure you're calling 2 callbacks in multiple places) and makes the .request() calls a little nastier, since you can't use trailing closures for completion without it throwing a warning.

[nbrooke]

I think it probably makes more sense to just only support the single argument Result callback, I don't think the additional complexity of having both forms is with the slight terseness gain of not needing to if case it when you do only need one.

If we are going to have multiple forms of request, I think API breadth is better spent on #64 and #77 which is likely more where the future of this sort of API is, and both can (potentially) get even terser.