Fix default command in interactive_open

[JesseWeinstein]

Fixes d5b7ce6

[nathdwek]

This construction looks like it could be simplified by simply redefining the function as

def interactive_open(targets, command = interactive_open()):

[JesseWeinstein]

That won't work because the command parameter is being passed, it is just being explicitly set to a falsey value. So using a default argument won't work.

[nathdwek]

Nice work! I totally agree on the re-assigning fix!

I just checked and it does indeed seem like shlex_split(open_anything) is equivalent to [open_anything()] (at least on my machine where open_anything() returns xdg-open but it seems that on all systems there's only one token in open_anything().

So basically to me it boils down to better readability but accepting to call shlex_split() on a string we already know is only one token, or a bit worse readability (in my opinion at least) but dealing with the case where no command is provided in the most efficient way.

Also I don't know how robust it is to have a function call inside another function definition, that could be a problem in my proposition.

[nathdwek]

As discussed in #1785, interactive_open() isn't called almost anywhere now, so I'm perfectly ok with changing its behaviour to make this fix more sensible.

I like the idea, I think it would maybe make more sense to simply use the right default value for command instead of None when initializing the config that is:

class PlayPlugin(BeetsPlugin):
    def __init__(self):
        [...]
        config['play'].add({
            'command': util.open_anything(),
            [...]
        })

Just from the top of my head, this seems a bit cleaner to me.

[JesseWeinstein]

But doesn't that leave open the possibility of crashing if the command is explicitly set to a falsey value?

[nathdwek]

From the user perspective, I don't really know what you're expecting if you put command: no in your config. That being said, I agree that beets should still handle this case gracefully and not explode in flight.

It seems that on a non string value, shlex_split() will fail in an ugly, non explicit way to the user.

nathdwek@nathMobile:~[master*=]$ vim .config/beets/config.yaml
nathdwek@nathMobile:~[master*=]$ beet play the wire
Playing 35 tracks.
Traceback (most recent call last):
  File "/usr/local/bin/beet", line 9, in <module>
    load_entry_point('beets==1.3.17', 'console_scripts', 'beet')()
  File "/usr/local/lib/python2.7/dist-packages/beets-1.3.17-py2.7.egg/beets/ui/__init__.py", line 1207, in main
    _raw_main(args)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.3.17-py2.7.egg/beets/ui/__init__.py", line 1197, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.3.17-py2.7.egg/beetsplug/play.py", line 130, in play_music
    util.interactive_open(open_args, command_str)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.3.17-py2.7.egg/beets/util/__init__.py", line 781, in interactive_open
    command = shlex_split(command)
  File "/usr/local/lib/python2.7/dist-packages/beets-1.3.17-py2.7.egg/beets/util/__init__.py", line 769, in shlex_split
    raise TypeError('shlex_split called with non-string')
TypeError: shlex_split called with non-string

If it's important to handle nonsensical configuration(which I think it is), then handling this would probably make the code uglier than the solution you propose now, even though the "flow" of the program would make more sense in the normal cases.

If you don't see a problem with this approach, and if nobody raises another point, I will probably merge this as is tomorrow in the evening.

E: That being said, imho util.open_anything() still makes more sense overall as a default value.

[sampsyo]

This looks good; thanks! And thanks for thinking through the error conditions. I don't have a particular opinion on what shows up in the default, but you're both right that it would be good to catch missing commands. I'm not 100% sure on the assert at the top of interactive_open—perhaps it would be the least surprising way to fail to say: "command '' not found" if you configure the plugin to look for a command called ''?

I'm going to write a brief changelog entry; can one or both of you please check to make sure it's correct?