require() only checks for existence of key, not empty value #702

Merged
merged 1 commit into from Aug 17, 2012

Projects

None yet

2 participants

@richid
Contributor
richid commented Aug 11, 2012

If the title is cryptic, I hope a simple example will explain:

from fabric.api import *

def staging():
    pass

def deploy():
    require('hosts', provided_by=[staging])

Running fab deploy will complete successfully even though hosts was never provided. This is because Fabric itself inserts an empty list at env['hosts'] and the require function only checks for the existence of the key, not the value. This is the code as of today:

missing_keys = filter(lambda x: x not in env, keys)

This can be improved by checking the value but would require some type-checking to ensure that values of 0 and False are handled appropriately, though it's not particularly Pythonic.

Modifying the lambda would look something like this:

missing_keys = filter(lambda x: (x in env and isinstance(env[x], (types.DictType, types.ListType, types.TupleType)) and not env[x]) or x not in env, keys)

It's probably better to use a simple for-loop as that lambda is abusive.

I've opened this issue without a patch to see if this is something you think should be pursued. If so, I'd be happy to write up a patch and corresponding tests.

I ran into the issue with the hosts key but I would expect that users who use require actually care that the value is not empty, not simply that the key exists.

In any event, thanks for the great package.

@bitprophet
Member

I think this is a reasonable change -- thanks for bringing it up!

I would probably tweak your modified lambda/loop a bit:

  • do the x not in env test first, so it can short-circuit right away if the key is actually missing
  • phrase the isinstance as isinstance(env[x], (dict, list, tuple) as the types module members are just aliases to the builtins in python 2, and don't even exist in python 3. So this is both shorter & more forwards compatible IMO.
@bitprophet
Member

Also, when you submit a pull request, I'd highly suggest using hub pull-request so it turns this issue into a pull request, instead of adding a 2nd issue. Less bookkeeping == better!

@richid richid Modify the require function to check values of keys in env dict
Currently, the require function only checks for the existence of the key
in the env dictionary, not the value. This change modifies that behavior
by checking the value of non-primitive types. That is, if a key exists
but its value is an empty list, dict, set, or tuple the require function
will throw an exception for that key. All other types are handled as
they were previously.
77bcd9f
@richid
Contributor
richid commented Aug 16, 2012

Thanks for the tip about removal of the types module in Py3, I really should invest some more time there...

@bitprophet
Member

To be honest I am still not using Python 3 at all myself -- but I am keeping an eye on changes to my OSS projects because the next iteration of them (incl Fabric) will attempt to be one of those 2.6=>3.x hybrid codebases.

In this case I couldn't remember if types was on the way in, or on the way out, so I checked it on Python 3's docs to see :)

@bitprophet bitprophet merged commit 528ad4a into fabric:master Aug 17, 2012
@bitprophet bitprophet added a commit that referenced this pull request Aug 17, 2012
@bitprophet bitprophet Changelog re #702 994a76e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment