I need to be able to accept some options from either the command-line or a config-file.
This code is a snippet of the class that I am working on, I cannot post the entire class here. This code works, but I am wondering if it can be made better. I will need to have a class here, there is a lot more work that goes on in the real code, I have pulled out just the parsing stuff. Please note, there are also several more sub-parsers that I will need, but the sub-parser shown is the only one that will possibly use a config file.
There is an issue with argparse with nesting argument_groups and mutually_exclusive_groups. So, I am not trying to use mutually-exclusive, but I am checking that with my own code in the functions.
import argparse
import yaml
from pprint import pprint
_possible_config_file_keys = ('names', 'ids')
class Parser(object):
def __init__(self):
self.build_parser()
self.options = self.parse_args()
#do other stuff here using self.options
def build_parser(self):
self.root_parser = argparse.ArgumentParser(description="Tool for ...")
self.root_sub_parsers = self.root_parser.add_subparsers(dest='action')
self.add_person_parser()
self.add_other_parser()
def add_person_parser(self):
parser = self.root_sub_parsers.add_parser('People')
g = parser.add_argument_group('People Targets')
g.add_argument(
"--config-file",
help='Supply a config file',
type=argparse.FileType(mode='r'))
g.add_argument('--name', dest='names', default=[], action='append')
g.add_argument('--id', dest='ids', default=[], action='append')
def add_other_parser(self):
pass #would do actual work here and add more subparsers
def parse_args(self):
args = self.parse_command_line_args()
if args.config_file:
self.parse_config_file(args)
delattr(args, 'config_file')
#I must ensure that 'Bob' is in the list of names
if 'Bob' not in args.names:
args.names.append('Bob')
return args
def parse_command_line_args(self):
return self.root_parser.parse_args()
def parse_config_file(self, args):
self.check_args(args)
data = yaml.load(args.config_file)
args.config_file.close()
for key, value in data.items():
if key not in _possible_config_file_keys:
self.root_parser.error("Invalid key '{}' in config file".format(key))
if isinstance(value, str):
value = [value, ]
setattr(args, key, value)
def check_args(self, args):
invalid_keys = [
key for key in vars(args).keys() if key in _possible_config_file_keys
and getattr(args, key)]
if invalid_keys:
self.root_parser.error("Cannot use config-file keys along with config-file")
#For my testing purpose
if __name__ == '__main__':
parser = Parser()
pprint(vars(parser.options))
1 Answer 1
It is good practice to set all of the instance attributes you need in __init__; moving across the contents of build_parser into __init__ would only add three lines!
It seems a bit... roundabout, in places. For example, __init__ calls parse_args which calls parse_command_line_args which calls the root_parser's parse_args. At the very least, I would roll the call to self.root_parser.parse_args into parse_args, as then you aren't going back and forth between method names as you follow the logic through.
There is an unexplained "magic string" in the middle of it all:
#I must ensure that 'Bob' is in the list of names
if 'Bob' not in args.names:
args.names.append('Bob')
The comment just says what the code is doing, but doesn't explain why, so it's redundant. In general, you have no explanations of the logic - docstrings would be helpful.
Some of those functions seem like implementation detail, rather than functionality you want to expose, so I would prefix the names with underscores to indicate private-by-convention, e.g. _add_person_parser and _add_other_parser.
I would make _possible_config_file_keys = ('names', 'ids') a class attribute of Parser, and indicate it as constant using the style guide naming conventions, i.e.
class Parser(object):
POSSIBLE_CONFIG_FILE_KEYS = ('names', 'ids')
...
You only seem to use pprint for testing, so I would move that import into the if __name__ == '__main__': block. Also, there should be a blank line between standard library and third-party imports:
import argparse
import yaml
Do you not need to check_args in cases where you don't parse_config_file? Again, some documentation to explain that logic would be helpful.