Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added optional argument for play-plugin #1532

Merged
merged 8 commits into from Aug 15, 2015
Merged

Added optional argument for play-plugin #1532

merged 8 commits into from Aug 15, 2015

Conversation

pheerai
Copy link
Contributor

@pheerai pheerai commented Jul 2, 2015

Summary:

  • New config key "play -> optargs"
  • New Subcommand argument "-o", "--optargs"
  • Set position using "{}" within "play -> command"-string

Motivation:
I use beets + play-plugin in connection with mplayer. Sometimes I want to listen to musicin the "natural" order, sometimes shuffled. As play doesn't provide such an option, I thought it would be helpful to implement the possibility to add (at request) some additional arguments to the command string.
Even though this could as well have been archived by shuffling the resulting playlist, I think this option gives more flexibility to the user. It can as well be used to route output to different output, etc.

How it works:
In the command-parameter within the config file, add "{}" to the position where you'd like to add the additional parameter (needed like that, as i.e. mplayers last option has to be "-playlist"), and add the config key "optargs: ". When calling beet play -o, the command will be called with inserted optional arguments.

I'm open for any kind of corrections and suggestions, as well as for a discussion whether or not this really should be merged.

 * New config key "play -> optargs"
 * New Subcommand argument "-o", "--optargs"
 * Set position using "{}" within "play -> command"-string
@sampsyo
Copy link
Member

sampsyo commented Jul 2, 2015

Interesting idea! I like this direction.

What do you think about this slightly different design: use an ordinary "pass-through arguments" deal. That is, you could use something like:

beet play --arg --random QUERY

and the --random flag would get passed through to the underlying command. You could still use a maker like {} to determine where to insert the new arguments, and we could also have a default, which would just append them to the end of the command (before the filename argument).

If you want to avoid typing --random every time, this should make it easy to make an alias.

@pheerai
Copy link
Contributor Author

pheerai commented Jul 5, 2015

Passing through arguments with possible defaults sounds reasonable. I'll take a look at this once the exams are over, but don't expect any masterpieces.

As for coding style, please give me some hints if anything doesn't suite.

@sampsyo
Copy link
Member

sampsyo commented Jul 6, 2015

Looking good so far! Thanks for following Travis' advice. 😃

@pheerai
Copy link
Contributor Author

pheerai commented Aug 1, 2015

Finally had some spare time and thought about your idea, @sampsyo .

Unfortunately, optparse doesn't support that right away by design (search
for “optional option”), so currently I have the following two approaches:

  1. Provide two arguments --useargs and --optargs, one allowing to use
    pre-specified arguments from the config, on to pass given options.
  2. Provide one argument with a mandatory option. If this contains another
    form-variable “{}” as used by format, the string specified in the
    config file will be inserted there. To just use the conf-file options, one
    has to use --optargs "{}".

My personal favorite (besides my first idea of replacing optparse with
argparse and using its nargs='?' feature) would be approach 2, as this
one gives more flexibility, and there won't be any confusion w.r.t. which
options to prefer and with which order to apply.

 * New argument `--optargs` reads string from option
 * If "{}" is present in the given string, the `optargs`
   from config-file get inserted at that point.
@sampsyo
Copy link
Member

sampsyo commented Aug 1, 2015

Thanks for investigating! This looks like a good plan to me, especially in the absence of an optional-argument feature. We can reexamine this when we move to Click (#1484).

In the mean time, can I ask you the nitpicky favor of changing the name to --args instead of --optargs? (Perhaps the short option can be -A, or -x to be reminiscent of Java's or Clang's -X arguments.) It's just that the arguments are of course optional, so saying beet play --args --foo seems to read more intuitively. Sorry for bikeshedding!

Other than that, we need to update the documentation and add a changelog entry. Then this will be ready to merge!

 * as discussed in #1532, with args-parameter, and optionally
   inserted config-key
 * updated changelog/docs
to make AppVeyor-builds possible.
@pheerai
Copy link
Contributor Author

pheerai commented Aug 3, 2015

Except I made the changelog-entry for version 1.3.14, this one's ready to merge.

@sampsyo sampsyo merged commit 9425811 into beetbox:master Aug 15, 2015
sampsyo added a commit that referenced this pull request Aug 15, 2015
Added optional argument for play-plugin
sampsyo added a commit that referenced this pull request Aug 15, 2015
sampsyo added a commit that referenced this pull request Aug 15, 2015
sampsyo added a commit that referenced this pull request Aug 15, 2015
@sampsyo
Copy link
Member

sampsyo commented Aug 15, 2015

I've merged a simplified version of this feature. Specifically, there's just a command-line flag now (no args: config option) and arguments are always appended to the end of the command (before the filename argument). I think this should be a little more foolproof: the latter should work because that's where most commands expect additional arguments, and the former should be OK because if you want to use the same arguments frequently, you can always make a shell alias. But let me know if you feel strongly about either of these being restored!

Thanks again for contributing this.

@pheerai
Copy link
Contributor Author

pheerai commented Aug 15, 2015

By simplyfing the way that the arguments get inserted into/onto the command-string, you actually make my own work useless for myself.

I'm using mpv (a fork of mplayer), and this player wants at most one times a "playlist"-argument directly in front of the playlist file. As specifying an optional argument with the simplified version puts it after the command-string, but in front of the playlist, mpv does'nt know what to do with the latter.

I would propose the following: Remove the pre-defined string, but leave the ability to position the argument. Thus, the feature would be usable for mpv/mplayer-people, but very much simplified.

sampsyo added a commit that referenced this pull request Aug 16, 2015
@sampsyo
Copy link
Member

sampsyo commented Aug 16, 2015

Thanks for the feedback. The above change restores the ability to insert arguments anywhere. By default, they're still tacked onto the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants