current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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))
share|improve this question

1 Answer 1

up vote 1 down vote accepted

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.