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

On Oct 10, 2012, at 04:34 PM, Robert Bruce Park 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.

That's different. In this latter case, you can be sure of the data type of
object you're accessing, but you can't be sure of the contents of the keys.
Of course, there are infinite number of unknown error conditions, so you can't
possibly defend against them all. Defending against missing keys is a
reasonable trade-off IMHO.

>> 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.

I think that's akin to trying to prove a negative. Taken to the extreme,
potentially any data type or value could be returned from get_json(), but it's
impossible to defend against them. As an example, it's probably entirely
possible that a non-empty list, or the integer 7 is returned from get_json(),
and in either of those cases, the original code would *still* fail because the
'or' clause wouldn't get triggered. It's only in the narrow case of a
non-true value (e.g. 0 or None, or the empty string) that the 'or' case would
get triggered.

In situations like this, I prefer to react to bugs so that we know exactly
what situation can cause the non-true return value to occur. In any event, a
test case would be useful.

You could argue that maybe in the .get() vs [] situation above we should do
the same - wait until a real-world bug arrives before we write defensive
code. That could be convincing, but the cost of adding defensive code in
those cases is so much higher because of the number of call sites that would
have to be fixed, so it seems like a better trade-off.
y