Use sub-commands instead of options for actions

elho commented

2019-06-16 18:38:21 +00:00

borgmatic uses options that are treated special for actions like create, list, etc.

This is generally unintuitive and even more so, as borg itself properly uses sub-commands (like many other tools like git do as well).
Furthermore, e.g. --list and --info are valid options to borg's create sub-command, adding extra confusion and making it akward to support them as options to borgmatic's create action.

Thus it would be more consistent (and two less characters to type 😉) if borgmatic used subcommands and e.g. borgmatic list did what one was meaning.

borgmatic uses options that are treated special for actions like create, list, etc. This is generally unintuitive and even more so, as borg itself properly uses sub-commands (like many other tools like git do as well). Furthermore, e.g. `--list` and `--info` are valid options to borg's `create` sub-command, adding extra confusion and making it akward to support them as options to borgmatic's create action. Thus it would be more consistent (and two less characters to type :wink:) if borgmatic used subcommands and e.g. `borgmatic list` did what one was meaning.

👍 1

thomasleveil commented

2019-06-16 21:55:11 +00:00

Contributor

I often forget to write the -- in front of borgmatic create. Having Borgmatic accept sub-command would make its usage closer to Borg's usage.

With Borg:

borg init
borg create
borg check
borg list
...

Today with Borgmatic:

borgmatic --init
borgmatic --create
borgmatic --check
borgmatic --list
...

I often forget to write the `--` in front of `borgmatic create`. Having Borgmatic accept sub-command would make its usage closer to Borg's usage. With Borg: - `borg init` - `borg create` - `borg check` - `borg list` - ... Today with Borgmatic: - `borgmatic --init` - `borgmatic --create` - `borgmatic --check` - `borgmatic --list` - ...

witten commented

2019-06-16 22:56:48 +00:00

Owner

The main reason borgmatic doesn't use "true" sub-commands is because:

borgmatic's default behavior is to run three logical actions together: --prune, --create, and --check.
With borgmatic's a la carte actions feature, a user can explicitly provide multiple actions in a single borgmatic invocation.

For both of these, as far as I know, there's not a way with Python's argparse for a user to specify multiple sub-commands in a single invocation. And conceptually, it makes these actions much less like traditional sub-commands and more like.. just flags. While Borg has the benefit of exposing single commands independently, borgmatic is a higher-level wrapper that bundles up multiple commands together.

Anyway, having said that, I'm open to brainstorming ways to change this existing behavior to get more towards the sub-command world.. Because I agree that it would be conceptually simpler if it's indeed possible. (I don't really like having to handle all of the edge cases of specifying multiple different actions together on a single invocation.)

As for @thomasleveil's idea of adding an alias of init for --init, create for --create, that would probably be possible without any major changes.

Thoughts on any of the above?

The main reason borgmatic doesn't use ["true" sub-commands](https://docs.python.org/3/library/argparse.html#sub-commands) is because: 1. borgmatic's default behavior is to run three logical actions together: `--prune`, `--create`, and `--check`. 2. With borgmatic's [a la carte actions](https://torsion.org/borgmatic/docs/how-to/deal-with-very-large-backups/#a-la-carte-actions) feature, a user can explicitly provide multiple actions in a single borgmatic invocation. For both of these, as far as I know, there's not a way with Python's `argparse` for a user to specify multiple sub-commands in a single invocation. And conceptually, it makes these actions much less like traditional sub-commands and more like.. just flags. While Borg has the benefit of exposing single commands independently, borgmatic is a higher-level wrapper that bundles up multiple commands together. Anyway, having said that, I'm open to brainstorming ways to change this existing behavior to get more towards the sub-command world.. Because I agree that it would be conceptually simpler if it's indeed possible. (I don't really like having to handle all of the edge cases of specifying multiple different actions together on a single invocation.) As for @thomasleveil's idea of adding an alias of `init` for `--init`, `create` for `--create`, that would probably be possible without any major changes. Thoughts on any of the above?

elho commented

2019-06-16 23:53:46 +00:00

Author

Personally, I'd work around the argparse limitation 1 by making a backup sub-command that invokes the 3 others.

But then, I'm biased anyway, IMO --prune (quite obviously with append-only repositories 😉) and --check are better to be scheduled independently anyway.

As for the aliases, that still doesn't solve any eventual support of options of the same name, ie. borgmatic create --list would do something quite different then borg create --list and you have to do away with the --list action to allow for a --list option to be added, for example.

Personally, I'd work around the `argparse` limitation 1 by making a `backup` sub-command that invokes the 3 others. But then, I'm biased anyway, IMO `--prune` (quite obviously with append-only repositories :wink:) and `--check` are better to be scheduled independently anyway. As for the aliases, that still doesn't solve any eventual support of options of the same name, ie. `borgmatic create --list` would do something quite different then `borg create --list` and you have to do away with the `--list` action to allow for a `--list` option to be added, for example.

elho commented

2019-06-17 00:09:40 +00:00

Author

Spontaneous, slightly hackish idea: Do a argparse parser with the common options and do add_argument('subcmds', metavar='COMMAND', nargs='+') then you split thesubcmds whenever there's an item not starting with a dash, ie. ['create', '--list', '--stats', 'check', 'list'] becomes ['create', '--list', '--stats'], ['check'], ['list'] and you then invoke the according separate argparse parsers (that otherwise would be sub-parsers) on each of the split lists.
You'd loose overall --help.

Spontaneous, slightly hackish idea: Do a `argparse` parser with the common options and do `add_argument('subcmds', metavar='COMMAND', nargs='+')` then you split the`subcmds` whenever there's an item not starting with a dash, ie. `['create', '--list', '--stats', 'check', 'list']` becomes `['create', '--list', '--stats'], ['check'], ['list']` and you then invoke the according separate `argparse` parsers (that otherwise would be sub-parsers) on each of the split lists. You'd loose overall `--help`.

witten commented

2019-06-19 18:07:28 +00:00

Owner

I don't think we actually need to support Borg's create --list directly, because borgmatic already rolls that up into the concept of a verbosity level. This is from borgmatic's create_archive() code:

        ...
        + (('--list', '--filter', 'AME-') if logger.isEnabledFor(logging.INFO) and not json else ())
        ...

And so I'm suggesting that --list doesn't need to be exposed directly to the user on the --create or create command-line.

Anyway, I think your general idea of splitting arguments and passing them to separate parsers is interesting. I'll have to play around with that. Or maybe without needing to support create --list, the alias idea would be good enough?

I don't think we actually need to support Borg's `create --list` directly, because borgmatic already rolls that up into the concept of a verbosity level. This is from borgmatic's `create_archive()` [code](https://projects.torsion.org/witten/borgmatic/src/branch/master/borgmatic/borg/create.py#L151): ```python ... + (('--list', '--filter', 'AME-') if logger.isEnabledFor(logging.INFO) and not json else ()) ... ``` And so I'm suggesting that `--list` doesn't need to be exposed directly to the user on the `--create` or `create` command-line. Anyway, I think your general idea of splitting arguments and passing them to separate parsers is interesting. I'll have to play around with that. Or maybe without needing to support `create --list`, the alias idea would be good enough?

elho commented

2019-06-20 06:03:56 +00:00

Author

create --list was just an example to make clear how to split and how having actions as positional arguments without dashes is unambiguous even if there is actions with the same names as options.

I don't see the aliases working. If you let argparse do it (be it it accepting longoption aliases without dashes, or you preprocessing argv and substituting any action with --action), it'll loose order and thus which option was meant to apply to which action and option verification (which the argparse sub-parsers would handle) will become way more complex, ie. refusing --create --foo -bar if --foo and --bar can not be combined within one operation (action), while allowing --create --foo --check --bar.

`create --list` was just an example to make clear how to split and how having actions as positional arguments without dashes is unambiguous even if there is actions with the same names as options. I don't see the aliases working. If you let argparse do it (be it it accepting longoption aliases without dashes, or you preprocessing `argv` and substituting any `action` with `--action`), it'll loose order and thus which option was meant to apply to which action and option verification (which the argparse sub-parsers would handle) will become way more complex, ie. refusing `--create --foo -bar` if `--foo` and `--bar` can not be combined within one operation (action), while allowing `--create --foo --check --bar`.

witten commented

2019-06-20 15:57:44 +00:00

Owner

Yeah, for the alias thing, I wasn't thinking of it as a full substitute for sub-commands, and more of just an aesthetic change. They'd still require all of the same option parsing code that's there now, so they wouldn't save us from that. Anyway, I'll have a look at the separate parser idea.

witten commented

2019-06-22 23:44:50 +00:00

Owner

I'm currently going with an approach with sub-commands where each sub-command's parser uses argparse's parse_known_args() on the full set of command-line arguments. This means that each sub-parser gets a whack at the full piñata of arguments. This isn't exactly what you asked for, so here are some pros and cons:

Pros:

Users can use more traditional sub-commands now, e.g. borgmatic create.
Still supports multiple "sub-commands" at once, e.g. borgmatic create --progress prune.
Allows arguments for a sub-command to be anywhere in the argument list: Examples that will work: borgmatic create --progress, borgmatic --progress create.
Allows global arguments to be anywhere in the arguments list. Examples that both work: borgmatic --verbosity 1 create, borgmatic create --verbosity 1.
100% backwards compatible. I think. E.g., this still works: borgmatic --create.
Much better code factoring since everything is now organized internally into sub-commands.
Nicer --help since things like borg create --help can show create-specific help.

Cons:

Doesn't support passing different instances of the same argument to different sub-commands. For instance, if --progress is anywhere on the command-line, all sub-commands that accept --progress will consume it. I actually think this might be less confusing, but it's different than your ask above.
Doesn't support your example of --list only getting passed to a single sub-command without triggering a borgmatic list action. (Sub-commands with the same name as actions.) I think this is a small price to pay though.
It's sort of an abuse of sub-commands / sub-parsers. They were certainly not designed for this.

I'm currently going with an approach with sub-commands where each sub-command's parser uses [argparse's parse_known_args()](https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.parse_known_args) on the full set of command-line arguments. This means that each sub-parser gets a whack at the full piñata of arguments. This isn't exactly what you asked for, so here are some pros and cons: Pros: * Users can use more traditional sub-commands now, e.g. `borgmatic create`. * Still supports multiple "sub-commands" at once, e.g. `borgmatic create --progress prune`. * Allows arguments for a sub-command to be anywhere in the argument list: Examples that will work: `borgmatic create --progress`, `borgmatic --progress create`. * Allows global arguments to be anywhere in the arguments list. Examples that both work: `borgmatic --verbosity 1 create`, `borgmatic create --verbosity 1`. * 100% backwards compatible. I think. E.g., this still works: `borgmatic --create`. * Much better code factoring since everything is now organized internally into sub-commands. * Nicer `--help` since things like `borg create --help` can show create-specific help. Cons: * Doesn't support passing different instances of the same argument to different sub-commands. For instance, if `--progress` is anywhere on the command-line, all sub-commands that accept `--progress` will consume it. I actually think this might be less confusing, but it's different than your ask above. * Doesn't support your example of `--list` only getting passed to a single sub-command without triggering a `borgmatic list` action. (Sub-commands with the same name as actions.) I think this is a small price to pay though. * It's sort of an abuse of sub-commands / sub-parsers. They were certainly not designed for this.

witten commented

2019-06-23 23:10:44 +00:00

Owner

This work has landed in master, and will be part of the next release. Feel free to play around with it and provide any feedback.

witten closed this issue

2019-06-23 23:10:44 +00:00

witten commented

2019-06-23 23:34:06 +00:00

Owner

Well, I decided to go ahead and release this. Mostly because docs now auto-publish from master, and I didn't want the online docs ahead of the released code! Anyway, you can grab borgmatic 1.3.9 and try this out.

real-or-random referenced this issue

2022-08-09 12:44:57 +00:00

Misleading arg name: borgmatic prune --files runs borg prune --list #571

Rows
Columns

Use sub-commands instead of options for actions #195