Repository: datalad/datalad

Issue:
Interface: Positional argument with dashes leads to an error
When converting the addurls plugin into a command, I wanted to use a positional argument that had a dash in it, but this doesn't seem to be possible.

As a simple example (which can be applied to master):

```diff
diff --git a/datalad/interface/__init__.py b/datalad/interface/__init__.py
index bd9019d36..cf2eff674 100644
--- a/datalad/interface/__init__.py
+++ b/datalad/interface/__init__.py
@@ -68,6 +68,7 @@
         ('datalad.interface.download_url', 'DownloadURL', 'download-url'),
         ('datalad.interface.run', 'Run', 'run'),
         ('datalad.interface.rerun', 'Rerun', 'rerun'),
+        ('datalad.interface.posarg', 'Posarg', 'posarg'),
     ])

 _group_plumbing = (
diff --git a/datalad/interface/posarg.py b/datalad/interface/posarg.py
new file mode 100644
index 000000000..6147b8f3b
--- /dev/null
+++ b/datalad/interface/posarg.py
@@ -0,0 +1,33 @@
+"""Take a position argument"""
+
+__docformat__ = 'restructuredtext'
+
+from datalad.distribution.dataset import datasetmethod
+from datalad.interface.base import Interface
+from datalad.interface.base import build_doc
+from datalad.interface.utils import eval_results
+from datalad.support.param import Parameter
+
+import logging
+
+lgr = logging.getLogger('datalad.interface.posarg')
+
+
+@build_doc
+class Posarg(Interface):
+    """Blah"""
+    _params_ = dict(
+        pos_arg=Parameter(
+            args=("pos-arg",),
+            doc="a positional argument.  Try to use me."),
+        opt_arg=Parameter(
+            args=("--opt-arg",),
+            doc="an optional argument"),
+    )
+
+    @staticmethod
+    @datasetmethod(name='posarg')
+    @eval_results
+    def __call__(pos_arg, opt_arg=None):
+        print(posarg)
+        yield {}

```

Invoking this results in the following error:

```
$ datalad posarg ok
[ERROR  ] 'Namespace' object has no attribute 'pos_arg' [base.py:<dictcomp>:393] (AttributeError)
```

If I replace `args=("pos-arg",)` with `args=("pos_arg",)`, I see the same error.

If I instead add `dest="pos_arg"`, I get a different error:

```
$ datalad posarg ok
Traceback (most recent call last):
  File "/home/kyle/src/python/datalad/venv-datalad/bin/datalad", line 8, in <module>
    main()
  File "/home/kyle/src/python/datalad/datalad/cmdline/main.py", line 304, in main
    parser = setup_parser(args or sys.argv)
  File "/home/kyle/src/python/datalad/datalad/cmdline/main.py", line 245, in setup_parser
    _intf.setup_parser(subparser)
  File "/home/kyle/src/python/datalad/datalad/interface/base.py", line 375, in setup_parser
    **parser_kwargs)
  File "/usr/lib/python3.5/argparse.py", line 1320, in add_argument
    raise ValueError('dest supplied twice for positional argument')
ValueError: dest supplied twice for positional argument
```

#### First error

I think what is going on is that `base.Interface.setup_parser` assumes that argparse replaces all dashes in the argument's name with underscores.  However, it seems that `ArgumentParser.parse_known_args` [leaves][0] dashes in positional arguments untouched:

```
Namespace(_=False, cfg_overrides=None, change_path=None, common_debug=False, common_idebug=False, common_on_failure=None, common_output_format='default', common_report_status=None, common_report_type=None, common_run_after=None, common_run_before=None, func=<bound method Interface.call_from_parser of <class 'datalad.interface.posarg.Posarg'>>, help=None, log_level='warning', logger=<logging.Logger object at 0x7f07c37babe0>, opt_arg=None, pbs_runner=None, pos-arg='ok', subparser=ArgumentParser(prog='datalad posarg', usage=None, description='Blah', formatter_class=<class 'argparse.RawDescriptionHelpFormatter'>, conflict_handler='error', add_help=False))
```

Note the "pos-arg".  So, when `base.Interface.call_from_parser` [goes looking][1] for `pos_arg`, it doesn't find it, leading to the attribute error.

Or, to remove this from the context of datalad:

```python
>>> import argparse
>>> parser = argparse.ArgumentParser()
>>> parser.add_argument('fo-o')
>>> parser.add_argument('--ba-z')
>>> parser.parse_args('arg'.split())
Namespace(ba_z=None, fo-o='arg')
```

#### Second error

The second error seems to come from the fact that `argparse.add_argument` treats `dest` [differently][2] depending on whether the argument is a positional argument or an optional one, but `Interface.base.setup_parser` doesn't account for this.

Fixing this error would make it easy to work around the first error by specifying `dest`.  If I'm thinking about this correctly, it should be enough for `setup_parser` to check whether the argument is a positional one and, if so, move `dest` from `parser_kwargs` to the first argument in `parser_args`.

[0]: https://github.com/datalad/datalad/blob/afe3db7e9fc01d84947452e33f8786cb953420cb/datalad/cmdline/main.py#L312
[1]: https://github.com/datalad/datalad/blob/afe3db7e9fc01d84947452e33f8786cb953420cb/datalad/interface/base.py#L393
[2]: https://docs.python.org/2/library/argparse.html?highlight=add_argument#dest


Discussion:

Patch:
diff --git a/datalad/cmdline/helpers.py b/datalad/cmdline/helpers.py
index 0ae39513c..b7a21922e 100644
--- a/datalad/cmdline/helpers.py
+++ b/datalad/cmdline/helpers.py
@@ -22,6 +22,7 @@ from textwrap import wrap
 from ..cmd import WitlessRunner as Runner
 from ..log import is_interactive
 from ..utils import (
+    ensure_unicode,
     getpwd,
     unlink,
 )
@@ -142,6 +143,73 @@ def parser_add_common_opt(parser, opt, names=None, **kwargs):
         parser.add_argument(*names, **opt_kwargs)
 
 
+def parser_add_common_options(parser, version):
+    parser_add_common_opt(parser, 'log_level')
+    parser_add_common_opt(parser, 'pbs_runner')
+    parser_add_common_opt(parser, 'change_path')
+    parser_add_common_opt(
+        parser,
+        'version',
+        version=f'datalad {version}\n')
+    if __debug__:
+        parser.add_argument(
+            '--dbg', action='store_true', dest='common_debug',
+            help="enter Python debugger when uncaught exception happens")
+        parser.add_argument(
+            '--idbg', action='store_true', dest='common_idebug',
+            help="enter IPython debugger when uncaught exception happens")
+    parser.add_argument(
+        '-c', action='append', dest='cfg_overrides', metavar='KEY=VALUE',
+        help="""configuration variable setting. Overrides any configuration
+        read from a file, but is potentially overridden itself by configuration
+        variables in the process environment.""")
+    parser.add_argument(
+        '-f', '--output-format', dest='common_output_format',
+        default='default',
+        type=ensure_unicode,
+        metavar="{default,json,json_pp,tailored,'<template>'}",
+        help="""select format for returned command results. 'default' give one line
+        per result reporting action, status, path and an optional message;
+        'json' renders a JSON object with all properties for each result (one per
+        line); 'json_pp' pretty-prints JSON spanning multiple lines; 'tailored'
+        enables a command-specific rendering style that is typically
+        tailored to human consumption (no result output otherwise),
+        '<template>' reports any value(s) of any result properties in any format
+        indicated by the template (e.g. '{path}'; compare with JSON
+        output for all key-value choices). The template syntax follows the Python
+        "format() language". It is possible to report individual
+        dictionary values, e.g. '{metadata[name]}'. If a 2nd-level key contains
+        a colon, e.g. 'music:Genre', ':' must be substituted by '#' in the template,
+        like so: '{metadata[music#Genre]}'.""")
+    parser.add_argument(
+        '--report-status', dest='common_report_status',
+        choices=['success', 'failure', 'ok', 'notneeded', 'impossible', 'error'],
+        help="""constrain command result report to records matching the given
+        status. 'success' is a synonym for 'ok' OR 'notneeded', 'failure' stands
+        for 'impossible' OR 'error'.""")
+    parser.add_argument(
+        '--report-type', dest='common_report_type',
+        choices=['dataset', 'file'],
+        action='append',
+        help="""constrain command result report to records matching the given
+        type. Can be given more than once to match multiple types.""")
+    parser.add_argument(
+        '--on-failure', dest='common_on_failure',
+        choices=['ignore', 'continue', 'stop'],
+        # no default: better be configure per-command
+        help="""when an operation fails: 'ignore' and continue with remaining
+        operations, the error is logged but does not lead to a non-zero exit code
+        of the command; 'continue' works like 'ignore', but an error causes a
+        non-zero exit code; 'stop' halts on first failure and yields non-zero exit
+        code. A failure is any result with status 'impossible' or 'error'.""")
+    parser.add_argument(
+        '--cmd', dest='_', action='store_true',
+        help="""syntactical helper that can be used to end the list of global
+        command line options before the subcommand label. Options taking
+        an arbitrary number of arguments may require to be followed by a single
+        --cmd in order to enable identification of the subcommand.""")
+
+
 def strip_arg_from_argv(args, value, opt_names):
     """Strip an originally listed option (with its value) from the list cmdline args
     """
diff --git a/datalad/cmdline/main.py b/datalad/cmdline/main.py
index bfe342ffd..8b2e4471d 100644
--- a/datalad/cmdline/main.py
+++ b/datalad/cmdline/main.py
@@ -32,7 +32,6 @@ from datalad.support.exceptions import CommandError
 from .helpers import strip_arg_from_argv
 from ..utils import (
     chpwd,
-    ensure_unicode,
     get_suggestions_msg,
     on_msys_tainted_paths,
     setup_exceptionhook,
@@ -100,71 +99,7 @@ def setup_parser(
         formatter_class=formatter_class,
         add_help=False)
     # common options
-    helpers.parser_add_common_opt(parser, 'log_level')
-    helpers.parser_add_common_opt(parser, 'pbs_runner')
-    helpers.parser_add_common_opt(parser, 'change_path')
-    helpers.parser_add_common_opt(
-        parser,
-        'version',
-        version='datalad %s\n' % datalad.__version__)
-    if __debug__:
-        parser.add_argument(
-            '--dbg', action='store_true', dest='common_debug',
-            help="enter Python debugger when uncaught exception happens")
-        parser.add_argument(
-            '--idbg', action='store_true', dest='common_idebug',
-            help="enter IPython debugger when uncaught exception happens")
-    parser.add_argument(
-        '-c', action='append', dest='cfg_overrides', metavar='KEY=VALUE',
-        help="""configuration variable setting. Overrides any configuration
-        read from a file, but is potentially overridden itself by configuration
-        variables in the process environment.""")
-    parser.add_argument(
-        '-f', '--output-format', dest='common_output_format',
-        default='default',
-        type=ensure_unicode,
-        metavar="{default,json,json_pp,tailored,'<template>'}",
-        help="""select format for returned command results. 'default' give one line
-        per result reporting action, status, path and an optional message;
-        'json' renders a JSON object with all properties for each result (one per
-        line); 'json_pp' pretty-prints JSON spanning multiple lines; 'tailored'
-        enables a command-specific rendering style that is typically
-        tailored to human consumption (no result output otherwise),
-        '<template>' reports any value(s) of any result properties in any format
-        indicated by the template (e.g. '{path}'; compare with JSON
-        output for all key-value choices). The template syntax follows the Python
-        "format() language". It is possible to report individual
-        dictionary values, e.g. '{metadata[name]}'. If a 2nd-level key contains
-        a colon, e.g. 'music:Genre', ':' must be substituted by '#' in the template,
-        like so: '{metadata[music#Genre]}'.""")
-    parser.add_argument(
-        '--report-status', dest='common_report_status',
-        choices=['success', 'failure', 'ok', 'notneeded', 'impossible', 'error'],
-        help="""constrain command result report to records matching the given
-        status. 'success' is a synonym for 'ok' OR 'notneeded', 'failure' stands
-        for 'impossible' OR 'error'.""")
-    parser.add_argument(
-        '--report-type', dest='common_report_type',
-        choices=['dataset', 'file'],
-        action='append',
-        help="""constrain command result report to records matching the given
-        type. Can be given more than once to match multiple types.""")
-    parser.add_argument(
-        '--on-failure', dest='common_on_failure',
-        choices=['ignore', 'continue', 'stop'],
-        # no default: better be configure per-command
-        help="""when an operation fails: 'ignore' and continue with remaining
-        operations, the error is logged but does not lead to a non-zero exit code
-        of the command; 'continue' works like 'ignore', but an error causes a
-        non-zero exit code; 'stop' halts on first failure and yields non-zero exit
-        code. A failure is any result with status 'impossible' or 'error'.""")
-    parser.add_argument(
-        '--cmd', dest='_', action='store_true',
-        help="""syntactical helper that can be used to end the list of global
-        command line options before the subcommand label. Options taking
-        an arbitrary number of arguments may require to be followed by a single
-        --cmd in order to enable identification of the subcommand.""")
-
+    helpers.parser_add_common_options(parser, datalad.__version__)
     # yoh: atm we only dump to console.  Might adopt the same separation later on
     #      and for consistency will call it --verbose-level as well for now
     # log-level is set via common_opts ATM
diff --git a/datalad/interface/base.py b/datalad/interface/base.py
index ade7c63d2..b117bdbfc 100644
--- a/datalad/interface/base.py
+++ b/datalad/interface/base.py
@@ -354,12 +354,12 @@ def update_docstring_with_parameters(func, params, prefix=None, suffix=None,
     # get the signature
     ndefaults = 0
     args, varargs, varkw, defaults = getargspec(func)
+    defaults = defaults or tuple()
     if add_args:
         add_argnames = sorted(add_args.keys())
         args.extend(add_argnames)
         defaults = defaults + tuple(add_args[k] for k in add_argnames)
-    if defaults is not None:
-        ndefaults = len(defaults)
+    ndefaults = len(defaults)
     # start documentation with what the callable brings with it
     doc = prefix if prefix else u''
     if len(args) > 1:
@@ -662,33 +662,49 @@ class Interface(object):
         # get the signature
         ndefaults = 0
         args, varargs, varkw, defaults = getargspec(cls.__call__)
-        if not defaults is None:
+        if defaults is not None:
             ndefaults = len(defaults)
+        default_offset = ndefaults - len(args)
+        prefix_chars = parser.prefix_chars
         for i, arg in enumerate(args):
             if not is_api_arg(arg):
                 continue
             param = cls._params_[arg]
-            defaults_idx = ndefaults - len(args) + i
+            defaults_idx = default_offset + i
             cmd_args = param.cmd_args
             if cmd_args == tuple():
                 # explicitly provided an empty sequence of argument names
                 # this shall not appear in the parser
                 continue
-            elif cmd_args is None:
-                cmd_args = []
-            if not len(cmd_args):
-                if defaults_idx >= 0:
-                    # dealing with a kwarg
-                    template = '--%s'
+
+            parser_kwargs = param.cmd_kwargs
+            has_default = defaults_idx >= 0
+            if cmd_args:
+                if cmd_args[0][0] in prefix_chars:
+                    # TODO: All the Paramteter(args=...) values in this code
+                    # base use hyphens, so there is no point in the below
+                    # conversion. If it looks like no extensions rely on this
+                    # behavior either, this could be dropped.
+                    parser_args = [c.replace('_', '-') for c in cmd_args]
                 else:
-                    # positional arg
-                    template = '%s'
-                # use parameter name as default argument name
-                parser_args = (template % arg.replace('_', '-'),)
+                    # Argparse will not convert dashes to underscores for
+                    # arguments that don't start with a prefix character, so
+                    # the above substitution must be avoided so that
+                    # call_from_parser() can find the corresponding parameter.
+                    parser_args = cmd_args
+            elif has_default:
+                # Construct the option from the Python parameter name.
+                parser_args = ("--{}".format(arg.replace("_", "-")),)
             else:
-                parser_args = [c.replace('_', '-') for c in cmd_args]
-            parser_kwargs = param.cmd_kwargs
-            if defaults_idx >= 0:
+                # If args= wasn't given and its a positional argument in the
+                # function, add a positional argument to argparse. If `dest` is
+                # specified, we need to remove it from the keyword arguments
+                # because add_argument() expects it as the first argument. Note
+                # that `arg` shouldn't have a dash here, but `metavar` can be
+                # used if a dash is preferred for the command-line help.
+                parser_args = (parser_kwargs.pop("dest", arg),)
+
+            if has_default:
                 parser_kwargs['default'] = defaults[defaults_idx]
             help = alter_interface_docs_for_cmdline(param._doc)
             if help and help.rstrip()[-1] != '.':
diff --git a/datalad/plugin/addurls.py b/datalad/plugin/addurls.py
index da56bf64f..31535983b 100644
--- a/datalad/plugin/addurls.py
+++ b/datalad/plugin/addurls.py
@@ -1027,7 +1027,7 @@ class Addurls(Interface):
             it ends with ".tsv". Otherwise, treat it as a CSV file.""",
             constraints=EnsureChoice(*INPUT_TYPES)),
         exclude_autometa=Parameter(
-            args=("-x", "--exclude_autometa"),
+            args=("-x", "--exclude-autometa"),
             metavar="REGEXP",
             doc="""By default, metadata field=value pairs are constructed with
             each column in `URL-FILE`, excluding any single column that is
@@ -1123,7 +1123,9 @@ class Addurls(Interface):
                  message=None, dry_run=False, fast=False, ifexists=None,
                  missing_value=None, save=True, version_urls=False,
                  cfg_proc=None, jobs=None, drop_after=False):
-        # Temporarily work around gh-2269.
+        # This was to work around gh-2269. That's fixed, but changing the
+        # positional argument names now would cause breakage for any callers
+        # that used these arguments as keyword arguments.
         url_file = urlfile
         url_format, filename_format = urlformat, filenameformat
 


Test patch:
diff --git a/datalad/interface/tests/test_base.py b/datalad/interface/tests/test_base.py
index b844cdd25..86d547f7e 100644
--- a/datalad/interface/tests/test_base.py
+++ b/datalad/interface/tests/test_base.py
@@ -10,10 +10,17 @@
 
 """
 
+import argparse
 import unittest.mock as mock
-from datalad.tests.utils import *
+from datalad.tests.utils import (
+    assert_in,
+    assert_not_in,
+    eq_,
+    ok_,
+    patch_config,
+    with_tempfile,
+)
 from datalad.utils import (
-    swallow_outputs,
     updated,
 )
 from datalad.cmd import (
@@ -26,6 +33,7 @@ from ..base import (
     nadict,
     nagen,
     NA_STRING,
+    update_docstring_with_parameters,
 )
 from argparse import Namespace
 
@@ -34,6 +42,10 @@ def _args(**kwargs):
     return Namespace(
         # ATM duplicates definitions done by cmdline.main and
         # required by code logic to be defined. (should they?)
+        #
+        # TODO: The common options are now added by
+        # cmdline.helpers.parser_add_common_options(), which can be reused by
+        # tests.
         **updated(
             dict(
                 common_output_format="default"
@@ -184,3 +196,52 @@ def test_status_custom_summary_no_repeats(path):
         protocol=StdOutCapture)
     out_lines = out['stdout'].splitlines()
     eq_(len(out_lines), len(set(out_lines)))
+
+
+def test_update_docstring_with_parameters_no_kwds():
+    from datalad.support.param import Parameter
+
+    def fn(pos0):
+        "fn doc"
+
+    assert_not_in("3", fn.__doc__)
+    # Call doesn't crash when there are no keyword arguments.
+    update_docstring_with_parameters(
+        fn,
+        dict(pos0=Parameter(doc="pos0 param doc"),
+             pos1=Parameter(doc="pos1 param doc")),
+        add_args={"pos1": 3})
+    assert_in("3", fn.__doc__)
+
+
+def check_call_from_parser_pos_arg_underscore(how):
+    from datalad.cmdline.helpers import parser_add_common_options
+    from datalad.support.param import Parameter
+
+    kwds = {"doc": "pos_arg doc"}
+    if how == "dest":
+        kwds["dest"] = "pos_arg"
+    elif how == "args":
+        kwds["args"] = ("pos_arg",)
+    elif how != "bare":
+        raise AssertionError("Unrecognized how: {}".format(how))
+
+    class Cmd(Interface):
+
+        _params_ = dict(
+            pos_arg=Parameter(**kwds))
+
+        def __call__(pos_arg, **kwargs):
+            return pos_arg
+
+    parser = argparse.ArgumentParser()
+    parser_add_common_options(parser, version="v1")
+    Cmd.setup_parser(parser)
+    args = parser.parse_args(["val"])
+    eq_(Cmd.call_from_parser(args),
+        "val")
+
+
+def test_call_from_parser_pos_arg_underscore():
+    for how in "bare", "dest", "args":
+        yield check_call_from_parser_pos_arg_underscore, how