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

using subcommands feels strange #17

Closed
RonnyPfannschmidt opened this issue Jun 5, 2012 · 32 comments
Closed

using subcommands feels strange #17

RonnyPfannschmidt opened this issue Jun 5, 2012 · 32 comments

Comments

@RonnyPfannschmidt
Copy link

having flags instead of some kind of lookup adds some strange loops to figure whats actually going on with the cli

@keleshev
Copy link
Member

keleshev commented Jun 5, 2012

Could you elaborate?

From docs:

You can instantly see that args['<name>'] is an argument, args['--speed'] is an option, and args['move'] is a command.

@RonnyPfannschmidt
Copy link
Author

the thing is that each subcommand gets an boolean value

for convient lookup something like a attribute to show the current subcommand would be nice

@keleshev
Copy link
Member

keleshev commented Jun 5, 2012

Your approach would be fine if there could only be one subcommand at the same time. But you can have many of them:

Usage:
  naval_fate.py ship new <name>...
  naval_fate.py ship [<name>] move <x> <y> [--speed=<kn>]
  naval_fate.py ship shoot <x> <y>
  naval_fate.py mine (set|remove) <x> <y> [--moored|--drifting]

If you have an API idea for return type—you are welcome.

@RonnyPfannschmidt
Copy link
Author

sounds like a good job for a list

an interesting way to chain tem for the given example could be something having each subcommand have the next command as value

that evaluates to true and can be chained
and the program name could refer to the first subcommans name

for naval_fate.py that would mean something like the following as result:

{ "naval_fate.py": "ship", "ship": "new", "new": True, "<name>": "example"}

@keleshev
Copy link
Member

keleshev commented Jun 5, 2012

if args[args["naval_fate.py"]] == "new" ? Or what usage case do you see?

@RonnyPfannschmidt
Copy link
Author

no, more something like :

#!python
command = args[':main']
func = getattr(cli, command)
app = make_app(args['--path'])
func(args, app)

@andreypopp
Copy link
Contributor

+1 on that — having activated command as an attribute value is more intuitive and useful to me

@keleshev
Copy link
Member

keleshev commented Jun 5, 2012

So you want to call a function with same name as the command? How about alternative discussed in #4 (kind of):

from docopt import command

@command 
def ship_new(args):
    """"Usage: naval_fate.py ship new ......."""

@command
def ship_move(args):
    """Usage: naval_fate.py ship [<name>] move .........""""

if __name__ == '__main__':
    command.run()

@RonnyPfannschmidt
Copy link
Author

then i'd have to split the docs again
right now i'll just go with my hack, i'll get back at it later

@mattbathje-xx
Copy link

What about something like this:

440,443d439
< class Func():
<     def __init__(self, value):
<         self.name = '__docoptfunc__'
<         self.value = value
445c441
< def docopt(doc, argv=sys.argv[1:], help=True, version=None, funcs=None):
---
> def docopt(doc, argv=sys.argv[1:], help=True, version=None):
455,463d450
<
<     cmds = frozenset([a.name for a in (pot_arguments+arguments)
<                       if type(a) == Command and a.value==True])
<
<     try:
<         func = [Func(funcs[cmds])]
<     except (KeyError, TypeError):
<         func = [Func(None)]
<
466c453
<                     (pot_options + options + pot_arguments + arguments + func))
---
>                     (pot_options + options + pot_arguments + arguments))

Which you then call from your code like so:

funcs={frozenset(['ship', 'new']):ship_new, frozenset(['ship', 'move']):ship_move, frozenset(['mine', 'remove']):mine_remove}
args = docopt(__doc__, funcs=funcs)
if args['__docoptfunc__']:
    args['__docoptfunc__'](args)

I used the weird name (__docoptfunc__) so that we can be very certain that it never conflicts with an actual argument/option that would be in the dict.

So the obvious extension of this is how to get it to only pass back that args/opts that go with the given command(s)? I haven't looked at this yet, but i'm wondering if there is a way to "filter" the list? If you think about it as a standard parse tree - we need to find the branch that contains all of the Commands given in funcs[cmd], and return only the args/opts on that branch. (At that point, we could go back to returning a tuple of (args, opts, func) if func is not None, or (args, opts) if func is None.)

@keleshev
Copy link
Member

@mattbathje I think your solution is more complicated than necessary. Compare:

funcs={frozenset(['ship', 'new']):ship_new, 
       frozenset(['ship', 'move']):ship_move, 
       frozenset(['mine', 'remove']):mine_remove}
args = docopt(__doc__, funcs=funcs)
if args['__docoptfunc__']:
    args['__docoptfunc__'](args)
args = docopt(__doc__)
if args['ship'] and args['new']:
    ship_new(args)
elif args['ship'] and args['move']:
    ship_move(args)
elif args['mine'] and args['remove']:
    mine_remove(args)

No clear advantage.

@Met48
Copy link
Contributor

Met48 commented Jun 15, 2012

What about a combination of the two?

@command('ship', 'move')
def ship_move(x, y, name=None, speed=None):
    pass

@command('ship', 'shoot')
def ship_shoot(x, y):
    pass

if __name__ == '__main__':
    command.run(__doc__, version='Naval Fate 2.0')

You'd still have one docstring and all the relevant arguments could be passed as kwargs. It could also simplify function re-use for related operations:

@command('mine', 'set|remove')  # or perhaps @command('mine', ('set', 'remove'))
def mine(x, y, set=False, remove=False, moored=None, drifting=None):
    if remove:
        print("Remove Mine:", x, y)
    else:
        print("Set Mine:", x, y)

@keleshev
Copy link
Member

@Met48 Kinda like your idea. Do you have any neat idea of API that would allow dispatch in both cases of 1 docstring and several docstrings?

@Met48
Copy link
Contributor

Met48 commented Jun 17, 2012

How's this?

"""Naval Fate

Usage:
  naval_fate.py ship new <name>...
  naval_fate.py mine (set|remove) <x> <y> [--moored|--drifting]

Options:
  ...

"""

from docopt import command


@command()
def ship_move(x, y, name=None, speed=None):
    """Usage: naval_fate.py ship [<name>] move <x> <y> [--speed=<kn>]"""


@command('ship new')
def ship_new(name):
    pass


@command('mine (set|remove)')
def mine(x, y, set=False, remove=False, moored=None, drifting=None):
    if remove:
        print("Remove Mine:", x, y)
    else:
        print("Set Mine:", x, y)


if __name__ == '__main__':
    command.run(__doc__, version='Naval Fate 2.0')

It differentiates between functions with their own docstring and functions for the main docstring. It also has the benefit of reusing the existing parser for subcommand matching.

@RonnyPfannschmidt
Copy link
Author

imho the proposals get more and more confusing and comlpex - i really think all that should be there is a way to discover the chain of commands without lots of bool checks and building the rest on top of that

@keleshev
Copy link
Member

@RonnyPfannschmidt do you have any example in mind?

@RonnyPfannschmidt
Copy link
Author

i think just puting a ':command' key into the dict that contains the ordered list of command parts would suffice

@keleshev
Copy link
Member

@RonnyPfannschmidt how about if docopt returned this kind of dictionary:

class Dict(dict):
    def __getitem__(self, key):
        if type(key) == tuple:
            return tuple(self[k] for k in key)
        return dict.__getitem__(self, key)

It is a backwards-compatible subclass of dict which you can use this way:

>>> args = Dict({'ship': True, 'move': True, 'new': False})
>>> args['ship']
True
>>> args['ship', 'move']
(True, True)
>>> all(args['ship', 'move'])
True
>>> any(args['ship', 'new'])
True
>>> all(args['ship', 'new'])
False

I.e. you can dispatch stuff as:

if all(args['ship', 'move']):
    ship_move()
elif all(args['ship', 'new']):
    ship_new()

What do you think?

@RonnyPfannschmidt
Copy link
Author

are you kidding?!

@keleshev
Copy link
Member

ok, looks like I'm very confused.

Anyway, I can't see how this could be used in practice:

# python naval_fate.py ship Guardian move 10 20 --speed=20
{'commands:': ['ship', 'move']
 '--drifting': False,
 '--help': False,
 '--moored': False,
 '--speed': '20',
 '--version': False,
 '<name>': ['Guardian'],
 '<x>': '10',
 '<y>': '20',
 'mine': False,
 'move': True,
 'new': False,
 'remove': False,
 'set': False,
 'ship': True,
 'shoot': False}

@RonnyPfannschmidt
Copy link
Author

it enables to take the list of commands in correct order and do name based lookup

that can be done to do stuff like getattr(cmdmod, '_'.join(args[':commands'])

also note the : in front, so it can be distinguished from the booleans
the important bit is to get the command strings in correct order, as those are required for any kind of lookup scheme that works with a convention of names of objects

@Met48
Copy link
Contributor

Met48 commented Jun 19, 2012

Why not add the :commands entry, then consider the other options? The :commands entry has general use for the programmer as it helps support any custom dispatch methods.

With that said, I think a more complete solution also needs to be present. The programmer should not have to maintain their own dispatch code if it can be avoided.

For comparison, here are all the mentioned dispatch methods, with docstrings and functions omitted:

#Current
if __name__ == '__main__':
    args = docopt.docopt(__doc__)
    if args['ship'] and args['add']:
        ship_add(args)
    elif args['ship'] and args['move']:
        ship_move(args)
    elif args['mine']:
        mine(args)

#Multi-match custom dict (with all() moved to the custom class)
if __name__ == '__main__':
    args = docopt.docopt(__doc__)
    if args['ship', 'add']:
        ship_add(args)
    elif args['ship', 'move']:
        ship_move(args)
    elif args['mine']:
        mine(args)

#Commands array with user-enforced naming scheme
#Does not support partial matches
class cmdmod(object):  # or an import of cmdmod

if __name__ == '__main__':
    args = docopt.docopt(__doc__)
    getattr(cmdmod, '_'.join(args[':commands']))

#Decorators for functions, with naming scheme or multiple docstrings
#Does not support partial matches
@command
@command
@command

if __name__ == '__main__':
    command.run()  # __doc__)

#Decorators for functions
@command('ship add')
@command('ship move')
@command('mine (set|remove)')

if __name__ == '__main__':
    command.run(__doc__)

#Function dictionary, modification of above frozenset method to simplify
#Does not support partial matches
if __name__ == '__main__':
    docopt.run(__doc__, funcs={
        ('ship', 'add'): ship_add,
        ('ship', 'move'): ship_move,
        ('mine', 'set'): mine,
        ('mine', 'remove'): mine,
    })

Of these options I think the decorator methods are the most minimal and flexible. Although the most complicated to implement, they aren't that confusing to use - take a look at Flask's routing for a similar example. The other methods have far more boilerplate or force naming conventions.

As an aside, if :commands is added perhaps a different prefix character could be chosen. ':commands' is far easier to mistake for 'commands' than alternatives like '@commands' and '_commands'.

@keleshev
Copy link
Member

@Met48 what do you mean by "partial matches"?

I don't like the ':command' solution, because:

  • it's a quirk and introduction of a reserved key-word:
    • take git where you delete remote branches using :name—whatever special character we introduce, they will likely make some crazy idea impossible
  • I don't think that getattr(cmdmod, '_'.join(args[':commands'])) is what most people want to do. If i need to implement naval_fate "for real" the code will likely be more of:
if args['ship'] and args['new']:
    ship = Ship(args['<name>'] or 'Mayflower')
elif args['ship'] and args['move']:
    ship.move(args['<x>'], args['<y>'])

how would having ':commands' help in this case?

I like the idea of having some kinda decorator-based dispatch much more. Almost sure, docopt will end up with something like @command('ship add'), but that does not help in the object-oriented example above, does it? Another problem with decorators—will be hard to translate them to other languages.

Since I'm for TSBOAPOOOWTDI, having both decorators and ':command' is redundant, that's why I'm not in a hurry to implement either of them.

The problems to be solved:

  • having several help-screens and showing them automatically (e.g. help <command>, and/or command --help)
    • probably function-docstrings are bad place for that:
      • shouldn't be limited to them at least
      • functions should be functions with their own signature and documentation
      • imagine any of git help screens—they are huge
  • dispatch
    • perfectly, a TSBOAPOOOWTDI-style dispatch
    • perfectly, dispatch that translates easily to Ruby/CoffeScript

@Met48
Copy link
Contributor

Met48 commented Jun 20, 2012

@halst By "partial matches" I mean being able to handle multiple subcommand combinations with the same function easily. I think this is worth having, as some operations may differ only slightly based on the subcommand.

I also think it is worth exposing the subcommands list through some means. If the dispatch method does not fit the needs of the programmer, there should be a fallback. To avoid the keyword problem, it could be an attribute on the returned Dict, or the module. It could also be exposed using the dispatch solution.

As for the dispatch solution, the @command decorators can handle the common cases. When the decorators are not sufficient, how about a solution similar to @mattbathje's: the module could expose a dispatch method that accepts a mapping of command filters to functions. This could also expose the subcommands list through a default function as follows:

import docopt

# ... Naval Fate functions ...

def other_command(commands):
    ...

if __name__ == '__main__':
    docopt.dispatch(__doc__, {
        'ship new': ship_new,
        'ship move': ship_move,
        None: other_command,
    })

Again, this would be a fallback for when decorators do not fit a more unusual use case. I am not sure how to best handle the TSBOAPOOOWTDI guideline here, but the decorator method could be prioritized as the obvious route.

Incorporating the help messages is possible with both dispatch solutions. The most straightforward would be as a second argument to the @command decorator or in a second mapping for the dispatch method.

As for the naval_fate example above, I'm not sure any dispatch solution would work for it. If there are only a few lines of code per subcommand, the dispatch code would be far longer than the bool checks already implemented. The nested checks are a more interested problem, one that should be better considered when exposing a dispatch method.

@RonnyPfannschmidt
Copy link
Author

maybe it is better if args got attributes instead of magic keys

so we can go and check directly for agrs.commands which would be a ordered tuple
this can also be used to build lookup maps just fine but would also fit for custom dispatch

after all the command decorator could just turn a expression into a number of tuples to register the function for them

@rubik
Copy link

rubik commented Jun 23, 2012

+1 for RonnyPfannschmidt solution. args could be a namedtuple or a namedtuple subclass and the problem would be solved...

@RonnyPfannschmidt
Copy link
Author

args is and must stay a dict subclass

@rubik
Copy link

rubik commented Jun 23, 2012

Why? And namedtuples have the _asdict() method.

@RonnyPfannschmidt
Copy link
Author

we have keys that start with -- or -, so normal attributes are a no go to begin with,
also its not really fit for a tuple, since it is a mapping to begin with

@rubik
Copy link

rubik commented Jun 24, 2012

Well, the namedtuple could have attributes that contains keys.

@RonnyPfannschmidt
Copy link
Author

please mock up a usage example just for the feel of it and a comparison

i think you will see that it will get more complicated for no real gain

@keleshev
Copy link
Member

keleshev commented Aug 1, 2012

This is essentially same as #4, further discussion moves there.

@keleshev keleshev closed this as completed Aug 1, 2012
mitkof6 pushed a commit to mitkof6/docopt that referenced this issue Oct 3, 2020
Write to supplied ostream instead of cout
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants