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

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

Except that some unforeseen error condition that prevents the dict from
being returned is *much* more likely to return None instead of the
integer 7 or a non-empty list, because None is what you get when you
call return with no values. So I don't think a simple test against the
false-ness of the return value is particularly unreasonable, especially
when it is only 5 bytes of source code.

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

Ok, but you made the change, so you write the test :-P