Open Stack

On 07/08/14 19:02, Kevin L. Mitchell wrote:
> On Thu, 2014-08-07 at 17:41 +0100, Matthew Booth wrote:
>> ... or arg is an object which defines __nonzero__(), or defines
>> __getattr__() and then explodes because of the unexpected lookup of a
>> __nonzero__ attribute. Or it's False (no quotes when printed by the
>> debugger), but has a unicode type and therefore evaluates to True[1].
> 
> If you're passing such exotic objects as parameters that could
> potentially be drawn from configuration instead, maybe that code needs
> to be refactored a bit :)
> 
>> However, if you want to compare a value with None and write 'foo is
>> None' it will always do exactly what you expect, regardless what you
>> pass to it. I think it's also nicer to the reviewer and the
>> maintainer,
>> who then don't need to go looking for context to check if anything
>> invalid might be passed in.
> 
> In the vast majority of cases, however, we use a value that evaluates to
> False to indicate "use the default", where "default" may be drawn from
> configuration.  Yes, there are cases where we must treat, say, 0 as
> distinct from None, but when we don't need to, we should keep the code
> as simple as possible.  After all, I doubt anyone would seriously
> suggest that we must always use something like the "_unset" sentinel,
> even when None has no special meaning…

I've found a few bugs in Openstack by checking implicit boolean tests
while reviewing code. Here's a recent one:

https://review.openstack.org/#/c/109006/1/nova/db/sqlalchemy/api.py

Note that the caller has accidentally passed read_deleted=False, a
pretty easy mistake to make, and the bare object test has hidden that
error and silently replaced it with a default.

Also note that while PEP8 stops short of decreeing against bare object
tests, it does recommend against it. See the section 'Programming
Recommendations':

http://legacy.python.org/dev/peps/pep-0008/

Matt
-- 
Matthew Booth
Red Hat Engineering, Virtualisation Team

Phone: +442070094448 (UK)
GPG ID:  D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490