play: Add prompt choice to importer #2360
Conversation
|
I fixed the appveyor issue. Can you rebase this on masterand push again? |
|
This is a cool idea; thanks! Any chance you could add some documentation on the feature? The refactoring also looks fine on first glance (I haven't done a full review yet). Anything in particular I should be looking for? (Or perhaps you can just give a brief overview of what needed changing?) |
fix line number add comments
diomekes
force-pushed
the
importer-play
branch
from
2c4aeeb
to
ab4246c
Compare
|
Yes, I forgot about adding documentation, I'll do that. I just defined separate methods/functions so that the CLI and event listener could access them. I did have to change the way the command is called for the importer prompt since the plugin originally would exit once the command was executed, but I needed it to return to beets. All my tests had it working fine. |
| """Returns either the raw paths of items or a playlist of the items. | ||
| """ | ||
| raw = config['play']['raw'].get(bool) | ||
| if raw: |
There was a problem hiding this comment.
This can be shortened to:
if config['play']['raw']:|
|
||
| def _print_info(self, selection, command_str, open_args, | ||
| item_type='track'): | ||
| """Prompts user whether to continue if playlist exceeds threshold. |
There was a problem hiding this comment.
I know this is minor, but docstrings should typically be written in the imperative voice—here, "Prompt" rather than "Prompts."
There was a problem hiding this comment.
Also, this docstring should probably explain what the return value means. Here, if I understand correctly, it returns true if the operation should be aborted (i.e., it returns false if the operation should actually happen, which is a little counterintuitive).
| open_args = paths | ||
| else: | ||
| open_args = [self._create_tmp_playlist(paths)] | ||
|
|
||
| self._log.debug(u'executing command: {} {!r}', command_str, open_args) |
There was a problem hiding this comment.
Maybe these logging messages should go in the method that actually plays the music?
| if warning_threshold and len(selection) > warning_threshold: | ||
| ui.print_(ui.colorize( | ||
| 'text_warning', | ||
| u'You are about to queue {0} {1}.'.format( | ||
| len(selection), item_type))) | ||
|
|
||
| if ui.input_options((u'Continue', u'Abort')) == 'a': | ||
| return | ||
| return True |
There was a problem hiding this comment.
The method should explicitly return False at the end (when it doesn't return True).
| @@ -155,3 +190,21 @@ def _create_tmp_playlist(self, paths_list): | |||
| m3u.write(item + b'\n') | |||
| m3u.close() | |||
| return m3u.name | |||
|
|
|||
| def before_choose_candidate_listener(self, session, task): | |||
There was a problem hiding this comment.
Argh; I wish we had chosen a better name than before_choose_candidate. 😱 But that's hardly your fault.
| adds a `plaY` option to the prompt, so pressing `y` will execute the configured | ||
| command and play the items currently being imported. | ||
|
|
||
| Once you exit your configured player, you will be returned to the import |
There was a problem hiding this comment.
Perhaps more accurately, "once the configured command exists, …"
| if not cancel: | ||
| play(command_str, paths, open_args) | ||
|
|
||
| def _create_command_str(self, args=None): |
There was a problem hiding this comment.
This can probably be called just _command_str.
| """ | ||
| try: | ||
| if keep_open: | ||
| command = command_str.split() |
There was a problem hiding this comment.
I'd recommend using beets.util.shlex_split instead of str.split for more predictable command spitting behavior.
|
Thanks for the thorough review, I think I addressed all the issues and hopefully the warning_threshold configuration option handling is a little more sophisticated. |
|
This looks great; thank you! I don't know why the 3.6 test failed the most recent time around, but I'm going to merge this now. To simplify things, I may remove that old, backwards-compatible misspelling… |
play: Add prompt choice to importer
This is for #2008.
I hope my refactoring isn't way out of line.