Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upMove argument validation outside of argparse #279
Conversation
This comment has been minimized.
This comment has been minimized.
codecov
bot
commented
Sep 11, 2018
•
Codecov Report
@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 91.63% 91.27% -0.37%
==========================================
Files 13 14 +1
Lines 1566 1581 +15
Branches 269 283 +14
==========================================
+ Hits 1435 1443 +8
- Misses 82 88 +6
- Partials 49 50 +1
Continue to review full report at Codecov.
|
|
A few minor concerns, mostly related to pre-3.6 compatibility. If you want to you can address them, otherwise I'll fix them before merging. I'll try to merge on Friday. As part of that I'll force-push a simplified commit history onto your pull request branch. |
| key in the GCOVR_CONFIG_OPTION_GROUPS dictionary. | ||
| """ | ||
|
|
||
| def __init__(self, name, *flags, **kwargs): |
This comment has been minimized.
This comment has been minimized.
latk
Sep 12, 2018
Member
Do we have to use **kwargs here, or can we enumerate all supported option-fields? Due to Python-2 restrictions we can't really combine *flags varargs with explicit keyword-only arguments, so we'd have to pass in an explicit list/tuple of flags. Something like:
def __init__(self, name, flags=(), group=None, help=None, action=None):
...
GcovrConfigOption('foo', ['--foo'], help="frobnicate something", action='store_true')… and so on, where everything after flags is intended to be used as a keyword-argument. This would allow us to get rid of some hasattr()/getattr() calls later, and clarifies the intended API.
| kwargs = dict(self.__dict__) | ||
| for key in ["name", "flags", "group"]: | ||
| del kwargs[key] | ||
| kwargs = ['{k}={v!r}'.format(k=k, v=v) for k, v in kwargs.items()] |
This comment has been minimized.
This comment has been minimized.
latk
Sep 12, 2018
Member
dict iteration order is undefined prior to Python 3.6. To get a stable repr() across runs we should sort the entries, e.g. [... for k, v in sorted(kwargs.items())].
| parser.add_argument(self.name, **kwargs) | ||
|
|
||
|
|
||
| GCOVR_CONFIG_OPTION_GROUPS = { |
This comment has been minimized.
This comment has been minimized.
latk
Sep 12, 2018
Member
dict order is undefined prior to Python 3.6. We can use collections.OrderedDict here so that the groups are always in the correct order
This creates an GcovrConfigOption class to handle the configuration. It was designed with argparse in mind but it can also be adapted to other systems. All config options are stored in the GCOVR_CONFIG_OPTIONS list. Initially there was also a class representing parsed values, so that we can check whether a value has been set. Instead, argparse.SUPPRESS is used to avoid a default assignment of None when the option has not been provided.
- explicitly specify all arguments instead of using kwargs - avoid unspecified dict iteration order prior to Python 3.6
latk
force-pushed the
stadelmanma:move-argument-validation-outside-of-argparse
branch
from
3e2bf79
to
d332597
The Travis/Python-3.4 build failed with an error within pkg_resources,
i.e. setuptools/pip infrastructure:
AttributeError: _DistInfoDistribution__dep_map
Online sources indicate updating pip first should help.
The problem is that pytest depends on setuptools, and also on other
modules that require up to date setuptools during their installation.
Since their installation order is undefined, installation can fail if
setuptools isn't updated first.
stadelmanma commentedSep 9, 2018
Extracts the command line api configuration out into a separate module (configuration.py). The
GcovrConfigOptionclasses store all of the necessary information to add themselves to an argparse parser and make that information easily accessible for any future configuration schemes. (The objects produced byadd_argumentare rather opaque.)Todo:
This is a building block for eventually addressing #229