Skip to content

assign values to argument namespace instead of parser #108

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

Merged
merged 3 commits into from
Sep 17, 2018

Conversation

fkloft
Copy link
Contributor

@fkloft fkloft commented May 2, 2018

Currently, metadata items are assigned to a dict that is stored as an attribute of the argument parser. The proper solution would be to assign the dict to the argument namespace, which is also the way the Python docs suggest.

Also, the generation of default values directly on the argument namespace is suppressed to not litter it with None values.

@coveralls
Copy link

coveralls commented May 2, 2018

Coverage Status

Coverage decreased (-82.04%) to 0.0% when pulling 55071cf on fkloft:argparser into db0e3dc on LibraryOfCongress:master.

@acdha
Copy link
Member

acdha commented Sep 17, 2018

It looks like most of the changed lines are whitespace-related. Can you run your branch through Black first?

Felix Kloft added 3 commits September 17, 2018 19:27
Python docs recommend to add properties to namespace, this allows for parsers to be reused.
attribute wouldn't be set without any metadata args. also removes the need to check for AttributeError
@fkloft
Copy link
Contributor Author

fkloft commented Sep 17, 2018

Sorry, this was an old PR. When I tried to merge the current master, I accidentally reverted some whitespace related changes from master. I rebased my commits onto master which fixed the whitespace issues.

@acdha acdha merged commit ab226af into LibraryOfCongress:master Sep 17, 2018
@fkloft fkloft deleted the argparser branch March 14, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants