Command-line parser issues about mech

[ColdHeat]

❯ mech init bento/ubuntu-16.04
Initializing mech
Loading metadata for box 'bento/ubuntu-16.04'
A `Mechfile` has been initialized and placed in this directory. You
are now ready to `mech up` your first virtual environment!

I just pulled the latest from the devel branch and it is indeed broken. Let me take a quick look.

[ColdHeat]

Hmm I'm not sure what @Kronuz was going for but I think

mech init [options] [<name>] [<url> | <path>]

This is the correct usage line.

[Kronuz]

name is either a vagrant cloud or a name to give the box if the second parameter is a url or a tar file. If no name not url/tar is given, an empty Mechbox gets created

[ColdHeat]

diff --git a/mech/mech.py b/mech/mech.py
index 4f643e8..09945c8 100644
--- a/mech/mech.py
+++ b/mech/mech.py
@@ -202,7 +202,7 @@ class MechBox(MechCommand):
         """
         Add a box to the catalog of available boxes.
 
-        Usage: mech box add [options] [<name> [<url|path>]]
+        Usage: mech box add [options] [<name>] [<location>]
 
         Notes:
             The box descriptor can be the name of a box on HashiCorp's Vagrant Cloud,
@@ -220,7 +220,7 @@ class MechBox(MechCommand):
                 --checksum-type TYPE         Checksum type (md5, sha1, sha256)
             -h, --help                       Print this help
         """
-        url = arguments['<url | path>']
+        url = arguments['<location>']
         if url:
             name = arguments['<name>']
         else:
@@ -526,7 +526,7 @@ class Mech(MechCommand):
         """
         Initializes a new mech environment by creating a Mechfile.
 
-        Usage: mech init [options] [<name> [<url|path>]]
+        Usage: mech init [options] [<name>] [<location>]
 
         Notes:
             The box descriptor can be the name of a box on HashiCorp's Vagrant Cloud,
@@ -545,7 +545,8 @@ class Mech(MechCommand):
                 --name INSTANCE              Name of the instance
             -h, --help                       Print this help
         """
-        url = arguments['<url | path>']
+        print(arguments)
+        url = arguments['<location>']
         if url:
             name = arguments['<name>']
         else:

Playing around with this a bit before I push it up.

[ColdHeat]

@juntalis can you pull from devel again? This should be fixed.

[Kronuz]

@ColdHeat, the very same is done a few other times in other places; I think we might want to change those too (and remove the print)

[Kronuz]

What were the arguments available anyway? Because I remember testing it and it worked. I wonder if something changed in docopt...

[juntalis]

Indeed, that did the trick for my Python 2 virtualenv. Still getting the same result in Python 3, but I'll play around with docopt a bit and see if I can work out what's going on there.

[juntalis]

Think I've figured it out. The non-breaking space character works as intended on Python 2, but not on Python 3. As a result, the init commands option patterns get compiled to the following:

Python 2

Required(
    Required(
        Optional(
            AnyOptions(
                Option('-f', '--force', 0, False),
                Option('-h', '--help', 0, False),
                Option(None, '--box-version', 1, None),
                Option(None, '--cacert', 1, None),
                Option(None, '--capath', 1, None),
                Option(None, '--cert', 1, None)
                Option(None, '--checksum', 1, None),
                Option(None, '--checksum-type', 1, None),
                Option(None, '--insecure', 0, False),
                Option(None, '--name', 1, None)
            )
        ),
        Optional(
            Argument('<name>', None)
        ),
        Optional(
            Argument('<location>', None)
        )
    )
)

Python 3

Required(
    Required(
        Command('init', False),
        Optional(
            AnyOptions(
                Option('-f', '--force', 0, False),
                Option('-h', '--help', 0, False),
                Option(None, '--box-version', 1, None),
                Option(None, '--cacert', 1, None),
                Option(None, '--capath', 1, None),
                Option(None, '--cert', 1, None),
                Option(None, '--checksum', 1, None),
                Option(None, '--checksum-type', 1, None),
                Option(None, '--insecure', 0, False),
                Option(None, '--name', 1, None)
            )
        ),
        Optional(
            Argument('<name>', None)
        ),
        Optional(
            Argument('<location>', None)
        )
    )
)

[Kronuz]

Umm... I knew that wan was going to give us problems some day... we may need to request a change to docopt, where we can pass the number of leading words which should be ignored to the parser (instead of it just assuming it has one and and ignoring only the first one), otherwise we will have to figure out some other character (will probe difficult in Python 3 unicode regexps, as I’m sure all spaces are flagged as space) or change the docs to only have the one word as the command doc (which is unfortunately ugly)

[Kronuz]

Third option could be using an entirely different character as separator and then somehow capture the usage exception so the character gets changed back to space before printing the usage to the user.

[Kronuz]

Or maybe simply monkeypatch DocoptExit.__init__() in command.py and wait till it breaks:

import docopt

NBSP = '__'

def DocoptExit____init__(self, message=''):
        SystemExit.__init__(self, (message + '\n' + self.usage.replace(NBSP, ' ')).strip())
docopt.DocoptExit.__init__ = DocoptExit____init__

[juntalis]

I've currently got it replacing the space with a dash, so the usage text reads mech-init, etc, but everything else works as expected.

If you're open to the idea of monkey patching docopt, you could always do something like:

@contextlib.contextmanager
def patched_docopt(name):
    # Join command names
    joined_name = name.replace(' ', '-')

    if name == joined_name:
        yield docopt
    else:
        # Store local references to patched functions.
        docopt_ref, extras_ref = docopt.docopt, docopt.extras
        
        # Create wrapper functions
        @functools.wraps(docopt_ref)
        def patched_docopt(doc, *args, **kwargs):
            doc = doc.replace(name, joined_name)
            return docopt_ref(doc, *args, **kwargs)
        
        @functools.wraps(extras_ref)
        def patched_extras(help, version, options, doc):
            doc = doc.replace(joined_name, name)
            return extras_ref(help, version, options, doc)
        
        # Patch docopt and yield
        docopt.docopt, docopt.extras = patched_docopt, patched_extras
        yield docopt
        
        # Revert patches
        docopt.docopt, docopt.extras = docopt_ref, extras_ref

# ...

    @staticmethod
    def docopt(doc, **kwargs):
        name = kwargs.pop('name', "")
        name = spaced(name)
        doc = textwrap.dedent(doc)
        with patched_docopt(name) as docopt:
            arguments = docopt.docopt(doc, options_first=True, **kwargs)
        return arguments

or a persistent patch of docopt.extras to replace mech-([^\s]+?) with mech \1.

[juntalis]

Actually, I'm not familiar with all the current usage scenarios of docopt, but what about just feeding in the expected command if name contains multiple words?

    @staticmethod
    def docopt(doc, **kwargs):
        argv = kwargs.pop('argv')
        name = kwargs.pop('name', "")
        name = spaced(name)
        commands = name.split(' ')
        doc = textwrap.dedent(doc)
        if len(commands) > 1:
            argv = commands[1:] + argv
        arguments = docopt.docopt(doc, argv=argv, options_first=True, **kwargs)
        return arguments

Scratch that - I see why the NBSP was necessary now.

[ColdHeat]

In my opinion, it seems like docopt development has slowed down.

Perhaps we should file a PR with docopt, and in the meantime monkey patch?

[juntalis]

In that case, I'll go ahead and use the monkey patching approach in my fork for the time being.

@Kronuz Looks like you have to patch both: docopt.DocoptExit.__init__ as well as docopt.extras. For some reason, -h and --help get handled in docopt.extras while -? uses the docopt.DocoptExit class. Ended up going with:

NBSP = '__'

def cmd_usage(doc):
    return doc.replace(NBSP, ' ')

docopt_extras_ref = docopt.extras
def docopt_extras(help, version, options, doc):
    return docopt_extras_ref(help, version, options, cmd_usage(doc))

def DocoptExit____init__(self, message=''):
    SystemExit.__init__(self, (message + '\n' + cmd_usage(self.usage)).strip())

docopt.extras = docopt_extras
docopt.DocoptExit.__init__ = DocoptExit____init__

[ColdHeat]

@juntalis do you want to PR your Python 3 changes? Happy to merge them in since I don't think docopt is going to be changing anytime soon. Seems like it would be better to switch to click at some point.

[Kronuz]

Should be fixed in v0.7.0+. Please re-open otherwise.