set should be permissible container for env.key_filename #610

Closed
brandon-rhodes opened this Issue Apr 5, 2012 · 2 comments

Projects

None yet

2 participants

@brandon-rhodes

Under Fabric 1.3.1, my fabfile was running wonderfully with env.key_filename being a Python set() of filenames. Having it be a set made my code simpler, since I could multiply-add filenames without worrying about creating duplicates. But with Fabric 1.4.1, this pattern now breaks, because the network.key_filenames() function has become more brittle — it makes the following check:

    # For ease of use, coerce stringish key filename into list
    if not isinstance(env.key_filename, (list, tuple)):
        keys = [keys]

If env.key_filename is here a set, then keys becomes a list with a set inside, which is not useful and raises an exception in the code that follows:

...
File ".../lib/python2.7/site-packages/fabric/network.py", line 150, in key_filenames
    return map(os.path.expanduser, keys)
  File ".../lib/python2.7/posixpath.py", line 252, in expanduser
    if not path.startswith('~'):
AttributeError: 'set' object has no attribute 'startswith'

Could the if statement quoted above either be expanded to allow for sets as well, or — even better — do a "positive" test for isinstance(env.key_filename, basestring) instead of a "negative" test against a list of a few container types? Thanks! :)

@bitprophet
Member

Yea, testing for basestring is definitely a better way to handle that (and is what the rest of the codebase tends to use in "string or iterable" tests) -- thanks for the catch.

@bitprophet bitprophet added a commit that closed this issue Apr 5, 2012
@bitprophet bitprophet Change env.key_filename test to support all iterables.
'is it a string or none' good, 'is it a list/tuple' bad.

Fixes #610
cd82b34
@bitprophet bitprophet closed this in cd82b34 Apr 5, 2012
@bitprophet
Member

For whatever reason, key_filename is also sometimes None and I couldn't be arsed to figure out exactly when/why that was considered a good thing. Easier to just assume that any non-iterable values will be strings or None. Herp derp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment