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

Add automatic support for `--format=` and `--path` to all commands #1271

Closed
brunal opened this Issue Jan 26, 2015 · 6 comments

Comments

Projects
None yet
3 participants
@brunal
Collaborator

brunal commented Jan 26, 2015

As announced in #1269 automatic support of --path and --format options is now going to be quite simple: such options will just have to set the config, there won't be any need to handle it in each plugin. This reduces code duplication and extends functionality.

As @sampsyo writes in #1269, beet ls -p feels more natural than beet -p ls, and shall be supported. This means adding the 2 options (-p and -f) to all subcommands. So, big question, should we replace current usages of -p and -f with path and format when there's a conflict? Pros: cleaner, simpler, more coherent. Con: changes extablished behaviour

Related issue: is it time to leave optparse for argparse? The former is deprecated since 2.7, the latter is more powerful. It has more intelligent built-in sub-commands support, which could simplify our problem: just add -p and -f as top-level options, they'll still be available & understood in sub-command options, but if there's a conflict at that level the sub-command option will win the fight (if I'm not mistaken).

@Kraymer

This comment has been minimized.

Collaborator

Kraymer commented Jan 26, 2015

should we replace current usages of -p and -f with path and format when there's a conflict?

-f is the --force flag for few plugins, does not seem reasonable to me to change that.
Could we consider adding only the long options (eg --path but no -p) to subcommands ?

@brunal

This comment has been minimized.

Collaborator

brunal commented Jan 26, 2015

Could we consider adding only the long options (eg --path but no -p) to subcommands ?

I'm not sure, I'll have to start hacking on it to get a real feel of the feasibility. Definitely sounds like a good idea. Otherwise beet -f "$myformat" plugin -f may be possible.

@sampsyo

This comment has been minimized.

Member

sampsyo commented Jan 26, 2015

Here's an alternative idea: make -f and maybe -p top-level flags, but provide aliases for them just for some commands. Maybe just the list command, where formatting is especially important—and other list-like commands, like beet random from the corresponding plugin. The idea is that beet ls -p is important because it matches intuition, but more obscure uses like beet mbsync -p are less important and beet -f '$path' mbsync works just fine if someone needs that.

This would also avoid naming conflicts in commands that don't currently have (and seem not to need) a new --format option.


On argparse: While I wholeheartedly agree that it would be great to move off of optparse, I have some experience with argparse, and I find it quite frustrating to work with. (I would rank it as slightly more frustrating than the old optparse but still less frustrating than getopt.) The 2.7 backport also lacks subcommand alias support (I contributed the feature to the 3.x version but the patch did not land in 2.x.) So I would advocate switching to Click instead, which is an absolute joy to use.

@Kraymer

This comment has been minimized.

Collaborator

Kraymer commented Jan 30, 2015

Here's an alternative idea: make -f and maybe -p top-level flags, but provide aliases for them just for some commands.

Sounds like a sweet compromise. 🍑

@brunal

This comment has been minimized.

Collaborator

brunal commented Mar 2, 2015

  • adding -p --path -f --format to every option unless there's a conflict is pretty easy in the end. However for many subcommands it is useless and it might clutter their --help message.
  • manual management of the format string in plugins is boring and error-prone. It should be set automatically once per session:
    • around beets.ui._raw_main() fetch the eventual top-level path & format options, or the selected subcommand options. Otherwise fallback on the list_format_*** config item
    • there'd be LibModel.set_format
    • subcommands should have a way to declare they're able to exploit custom formatting so they get -p, -f & co. added as subcommand-level options, and fetched around _raw_main()
    • one trouble with automatic management of the format is that one cannot know outside the plugin whether the format is intended for items or albums
@sampsyo

This comment has been minimized.

Member

sampsyo commented Mar 2, 2015

I agree about cluttering help messages and such—it seems like it would be easier (and more intelligible?) to provide --format and perhaps --album-format as global options. The list command could perhaps be an (the only?) exception to make it less surprising. At the moment, I can't think of other commands that I'd advocate for adding -f and -p flags to, except for backwards compatibility.

The canonical beets way to override the configured formats would be to just set them in the configuration structure. So the LibModel classes would look up their formats by using config['format_item'] and config['format_album'] rather than needing to have additional class-level state. The idea is to encapsulate all the configuration state in one big structure as much as possible rather than letting it leak into individual fields throughout the core. (Although we do already violate this plenty…)

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