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