# Partial implementation of parsing a config-file and command line options

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 ...")

"--config-file",
help='Supply a config file',
type=argparse.FileType(mode='r'))

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)
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))


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.