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

interface: Allow positional arguments with underscores #5525

merged 8 commits into from Mar 24, 2021


Copy link

@kyleam kyleam commented Mar 23, 2021

This series updates the argument parsing logic so that parameter names for positional options can use underscores. The third commit contains a fix for a type bug that I ran into while testing this. The last commit contains the main fix.

Closes #2269.

cc @christian-monch

When processing its add_args value, update_docstring_with_parameters()
assumes getargspec returns a tuple for "defaults", but it returns None
when there are no optional arguments.

This doesn't matter in practice for DataLad commands because they all
have optional arguments.
It doesn't functionally matter, but go with the PEP-recommended
This ends up as --exclude-autometa on the command line because
Interface.setup_parser() maps this to a dash, so it's confusing to use
an underscore here.  (And this is the only option in the code base
that uses an underscore in an `args` value.)
Some interface/parser logic expects these common options to be
present, which makes testing it more cumbersome.  Move the code for
adding these options out of main.setup_parser() so that tests can more
easily use it.
Interface.setup_parser() assumes that argparse replaces all dashes in
the argument's name with underscores. However, argparse leaves dashes
in positional arguments untouched.  This results in an error when
positional arguments specified via Parameter() use names with
underscores (in the Python variant) and dashes (in the command-line

Trying to work around this by using Parameter(..., dest=...) just
leads to a different error because Interface.setup_parser() doesn't
account for the fact that argparse.add_argument() treats `dest`
differently depending on whether the argument is a positional argument
or an optional one.

Teach setup_parser() to 1) leave positional arguments untouched and 2)
not pass `dest` as a keyword argument to add_argument() when adding a
positional parameter.  Parameter names for positional argument can now
include underscores, either inferred from the parameter name or
explicitly specified via `args`.  If the caller wants the dashed form
to show up in the command-line output, `metavar` can be specified.

Closes datalad#2269.
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #5525 (5320068) into maint (0762df0) will decrease coverage by 0.61%.
The diff coverage is 98.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5525      +/-   ##
- Coverage   90.19%   89.58%   -0.62%     
  Files         296      296              
  Lines       41989    42020      +31     
- Hits        37871    37642     -229     
- Misses       4118     4378     +260     
Impacted Files Coverage Δ
datalad/plugin/ 92.72% <ø> (-4.34%) ⬇️
datalad/interface/tests/ 99.15% <96.66%> (-0.85%) ⬇️
datalad/cmdline/ 69.86% <100.00%> (+2.94%) ⬆️
datalad/cmdline/ 75.00% <100.00%> (-1.14%) ⬇️
datalad/interface/ 91.42% <100.00%> (+0.04%) ⬆️
datalad/support/ 48.00% <0.00%> (-32.00%) ⬇️
datalad/ui/ 56.25% <0.00%> (-21.88%) ⬇️
datalad/metadata/extractors/tests/ 87.80% <0.00%> (-12.20%) ⬇️
datalad/metadata/extractors/tests/ 88.46% <0.00%> (-11.54%) ⬇️
datalad/metadata/extractors/tests/ 88.46% <0.00%> (-11.54%) ⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0762df0...5320068. Read the comment docs.

Copy link

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, and thx much for the batch of clean-ups on the side!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

T H A N K Y O U !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there may be only four files left that use a star import :)

@kyleam kyleam merged commit bc80aba into datalad:maint Mar 24, 2021
@kyleam kyleam deleted the posarg-underscore branch March 24, 2021 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants