Raise errors by default

[timbertson]

I haven't written any code for this yet, but I've been thrown off a bunch of times by how this client treats errors. Most methods will fail silently, and appear to do something reasonable:

(pry)> client.apps.insatllations.to_a
=> []

The typo above means I think I've got no apps installed, while actually the client is swallowing a HTTP 400. I've also encountered code which create()s an oauth token and continues on, without noticing that the return value is nil and the creation failed due to a HTTP 403 (without logging any error / warning).

I'm sure I could check some errors object, but "perform action; check for errors" is not a very convenient way to code - and I suspect most people wouldn't think of this until they get bitten at least one by this behaviour.

So I'd like to propose a (gradual) change in how we deal with errors in the API client.

Phase 1: explicit, opt-in

For each error-raising method, we currently have foo() (supresses errors) and foo!() (raises errors). We will also add try_foo() which suppresses errors, so that you can explicitly raise or ignore errors.

The existing foo() methods will continue to suppress errors (just like try_foo()) by default. However, when constructing a client we'll also allow users to specify:

config.raise_errors = true

This will make all foo() methods act like foo!() instead of try_foo().

We can also instruct people to specify:

config.raise_errors = false

If they want to explicitly keep the current error handling behaviour.

Phase 2: loud by default

After the above code has had a while for people to code to the new opt-in behaviour, I think we should change the default raise_errors setting to be true. This would be a backwads-incompatible change, but I believe it's worthwhile since the current behaviour causes fragile code and confusing behaviour.

Phase 3: cleanup

If we (and users) are convinced that the new error behaviour is better, we should remove the raise_errors config option and get rid of the old behaviour entirely.

---

I'd definitely like to implement at least phase 1, since that's compatible with all existing code. But I'd like feedback on all of it as I'm sure people have opinions on this :)

/cc @steved

[steved]

I'm going to agree, because this is the way I've personally seen everyone implement their integrations. However, I don't think using exceptions as control flow is the correct design decision, even if python and ruby use it extensively.

I'm sure I could check some errors object, but "perform action; check for errors" is not a very convenient way to code - and I suspect most people wouldn't think of this until they get bitten at least one by this behaviour.

This strikes me as a little flippant. This should be the way to properly implement external systems.

If I had all the time in the world, I would rewrite most of the client. Zendesk's REST API has become too big and too varied for this implementation. I'm going to spend a little time thinking about it and see if there isn't a way to bridge my hopes and dreams with your proposal here.

[timbertson]

Glad to hear you're in agreement, and my apologies for being flippant. I do stand by my conclusion though, that most people (in ruby) won't realise that they need to code this way until something goes wrong.

This should be the way to properly implement external systems.

I'm not totally sure what "This" you're referring to here. If you're talking about not using exceptions for control flow, I think there's something to that - but as you say, it's not done much in practice. I think one reason for that is that (in ruby) it becomes difficult to prevent people from using it wrong, whereas exceptions can't accidentally be ignored.

I'll hold off implementing any of this are until you've had a chance to think about where you want it to head, feel free to ping me if you'd like to discuss anything :)

[steved]

I think one reason for that is that (in ruby) it becomes difficult to prevent people from using it wrong, whereas exceptions can't accidentally be ignored.

As much as I dislike it, implementing for the lowest denominator is the right decision.

I think from an implementation standpoint, one problem to mull is that lazy loaded collections and exceptions might not be able to coexist within the "principle of least surprise." For the basic instance CRUD actions, slowly deprecating and removing foo() for foo!() works. Collections might need to integrate a .lazy type modifier.

Either way, fixing the documentation is the first step: #282

[timbertson]

Definitely 👍 on the docs. I'm not quite sure what you mean about the lazy collections, though. Everything is lazy currently, right? So I'd imagine you could call .getSomeCollection() without issue, you'd just get the exception as soon as you forced the collection to be resolved (via .each, .to_a, etc)?

[steved]

Right, I just think we have to choose one world and stick to it. Laziness work nicely with deferred and up-front error handling. Exception handling should be as local to the execution as possible, which is much harder when passing a lazy collection around.