Support empty list in arguments list?

[joshma]

Was hoping to better understand why there's a warning when you try eg

sh.echo('foo', [], 'bar')
__main__:1: UserWarning: Empty list passed as an argument to '/bin/echo'. If you're using glob.glob(), please use sh.glob() instead.

My use case is I want to pass in options only sometimes:

options = [] if condition else ['-v', 'option']
sh.mycommand('foo', options, 'bar')

Right now this warns - is there a better way to accomplish my use case?

[amoffat]

In [4]: glob.glob("*.fjoae")
Out[4]: []

In [5]: sh.glob("*.fjoae")
Out[5]: '*.fjoae'

A glob.glob that doesn't expand to anything is an empty list, which make commands have misleading output or error messages when people use it with sh commands. For example:

sh.ls(glob.glob("*.fjoae"))

Will print the contents of your home directory, because you've effectively called ls with no args. Other commands behave more unexpectedly.

sh.glob just returns the pattern you passed in, if it would expand to an empty list, so your error message would be like:

/bin/ls: cannot access *.fjoae: No such file or directory

Throwing a warning on an empty list isn't the greatest solution in the world, but it solves more problems that it causes. For your case, you should probably do something like:

condition = True
options = [] if condition else ['-v', 'option']
options.append("bar")
sh.mycommand('foo', *options)

Or you can suppress warnings. Or if you have other ideas on how to better handle the glob situation, we can talk those through too.

[joshma]

Hm, that's rough. What if glob returned a subclass of list that sh.Command knows about? It could treat an empty globlist with a warning.

[amoffat]

You mean monkey-patching glob.glob? I'm not totally opposed to it actually

[joshma]

Ah, totally forgot that glob was in stdlib. Monkey-patching scares me, seems like a lot of mess for somewhat of an edge case.

I think my main issue is that the warning is pretty confusing if you're not using glob - maybe it can just be rewritten to indicate you have nothing to worry about if you're not using it / if you're not doing the equivalent of glob.

On the other hand, I'd personally prefer if it just didn't warn, especially since the docs recommend using sh.glob...

[amoffat]

idk, I'm kind of liking the monkey patch idea, and it's all your fault :)

I'm thinking something like:

import glob

old_glob = glob.glob

def patched_glob(*args, **kwargs):
    expanded = old_glob(*args, **kwargs)
    expanded._sh_glob_list = True
    return expanded

glob.glob = patched_glob

then the sh logic becomes: if list is empty and has _sh_glob_list attribute, output warning, otherwise use list as normal. i honestly don't see it as too terrible, and any biting it could do would only be related to someone somehow getting an empty list from glob that doesn't have _sh_glob_list on it. but they would have to jump through hoops to get that, like copying the list into a new list, or obtaining a reference to glob.glob before it is patched

I know if I were to remove the warning entirely, I would get people opening issues again that their glob.glob() code isn't working right.

[joshma]

It's a bit magical - I'd get confused if I thought about it and tried to figure out why an empty list from glob.glob() was being treated differently.

However, assuming it's more common that users of the library get bitten by an empty array, since that doesn't really mirror shell behavior (eg ls *.py), I can see the argument for treating glob.glob() as using that sort of construct and expecting the same shell-like behavior.

At the risk of supporting monkey-patching in public, I'm okay with the glob patching as discussed.

[amoffat]

Monkey patching glob solution in release-1.2 branch, which should fix the original issue when it goes out