Code review comment for lp:~robru/gwibber/locked-login-refactor

Revision history for this message
Robert Bruce Park (robru) wrote :

On 12-10-10 09:38 AM, Barry Warsaw wrote:
> This looks great, and I'm ready to merge it, but I have one question.
>
>> - data = get_json(SELF_URL.format(access_token=token)) or {}
>
> I see this pattern a lot, but I wonder why. Is it expected that get_json()
> would return a false value that *wasn't* an empty dictionary?

Well, just like the millions of places where I write .get('foo') instead
of ['foo'], it's written that way to deliberately be fault-tolerant
against *unknown* error conditions.

> I'm very tempted to take out the "or {}" unless there's a concrete reason
> to add it back (read: failing test :).

Well, if you want to make the code *more* brittle, I think it's up to
you to point at some API reference documentation that indicates that the
return value will *always* be some JSON dict regardless of any error
condition that might arise. I think the onus is on you to prove that
your change is safe, not on me to prove that this clearly-harmless edge
case won't save our butts at some point.

« Back to merge proposal