New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite argument parsing #4493

Open
bmw opened this Issue Apr 12, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@bmw
Copy link
Contributor

bmw commented Apr 12, 2017

We currently use the largely unmaintained configargparse and a number of hacks like magic default values and reaching into the internals of configargparse/argparse to get the behavior we want. We should rewrite our argument parsing to use another library like click or fire.

cc @erikrose, @ohemorange

@bmw bmw added this to the Wishlist milestone Apr 12, 2017

@erikrose

This comment has been minimized.

Copy link
Collaborator

erikrose commented Apr 12, 2017

I've had excellent experiences with click. See https://github.com/mozilla/dxr/blob/master/dxr/cli/__init__.py for the top-level entrypoint in DXR's click-based CLI. click is particularly good at subcommands.

@ohemorange

This comment has been minimized.

Copy link
Contributor

ohemorange commented Apr 12, 2017

I don't know anything about click, but my friend wrote fire so we could probably get good support. I'm all for cleaner code everywhere!

@erikrose

This comment has been minimized.

Copy link
Collaborator

erikrose commented Apr 12, 2017

Then we should probably make a pro/con list and see what pops out! Nice to have 2 good choices.

@anarcat

This comment has been minimized.

Copy link

anarcat commented Jan 15, 2019

i don't know if click will cover the use cases currently covered by configargparse. first, it doesn't support config files out of the box - that might be done by a third party module like click_config_file. that config file support doesn't support writing config files the way certbot currently does either.

then click is way more than a commandline parser - it's a whole TUI (text user interface) driver. it hijacks the main control flow of your program and makes testing ... different, for example. it supports progress bars and ANSI art and whatnot. it is very opiniated about how things should be and you might find it difficult to work with if you deviate from canon.

fire is much simpler, but also has the limitation of not working with config files.

another option people often favor is docopt which also doesn't support config files (let alone writing them) but there is some sample code to parse config files that is surprisingly clean.

there aren't that many options here - rewriting the commandline parser is a huge deal in certbot. argparse, while it's weird and complicated, is one of the best things we have right now. i wonder if the best approach might not be to fix the issues in configargparse instead of rewriting half of certbot. ;) and there's been some action on the issue of finding a new maintainer on that side, so I wouldn't give up on it already...

@bmw

This comment has been minimized.

Copy link
Contributor Author

bmw commented Jan 15, 2019

Thanks for the thoughts @anarcat! For what it's worth, we don't use configargparse for writing configuration files and instead use configobj.

@anarcat

This comment has been minimized.

Copy link

anarcat commented Jan 15, 2019

well that makes things easier. for what it's worth, I ended up ditching all those methods for my own implementation and just rolled my own. I suggest you take that into consideration...

https://gitlab.com/anarcat/undertime/blob/acd4a7c73f7e9c727a8a572884a46bcb696682c9/undertime#L102

it's around 60 lines of code, but i think it's quite elegant, all things considered. it uses pyyaml to parse the config file, but that's easy to override... heck, it's short enough that I can just paste it here:

class ConfigAction(argparse.Action):
    """add configuration file to current defaults"""
    def __init__(self, *args, **kwargs):
        """the config action is a search path, so a list, so one or more argument"""
        kwargs['nargs'] = '+'
        super().__init__(*args, **kwargs)

    def __call__(self, parser, ns, values, option):
        """change defaults for the namespace, still allows overriding
        from commandline options"""
        for path in values:
            parser.set_defaults(**self.parse_config(path))

    def parse_config(self, path):
        """abstract implementation of config file parsing, should be overriden in subclasses"""
        raise NotImplementedError()


class YamlConfigAction(ConfigAction):
    """YAML config file parser action"""
    def parse_config(self, path):
        try:
            with open(os.path.expanduser(path), 'r') as handle:
                return yaml.safe_load(handle)
        except (FileNotFoundError, yaml.parser.ParserError) as e:
            raise argparse.ArgumentError(self, e)


class ConfigArgumentParser(argparse.ArgumentParser):
    """argument parser which supports parsing extra config files

    Config files specified on the commandline through the
    YamlConfigAction arguments modify the default values on the
    spot. If a default is specified when adding an argument, it also
    gets immediately loaded.

    This will typically be used in a subclass, like this:

            self.add_argument('--config', action=YamlConfigAction, default=self.default_config())

    """

    def _add_action(self, action):
        # this overrides the add_argument() routine, which is where
        # actions get registered. it is done so we can properly load
        # the default config file before the action actually gets
        # fired. Ideally, we'd load the default config only if the
        # action *never* gets fired (but still setting defaults for
        # the namespace) but argparse doesn't give us that opportunity
        # (and even if it would, it wouldn't retroactively change the
        # Namespace object in parse_args() so it wouldn't work).
        action = super()._add_action(action)
        if isinstance(action, ConfigAction) and action.default is not None:
            # fire the action, later calls can override defaults
            try:
                action(self, None, action.default, None)
            except argparse.ArgumentError:
                # ignore errors from missing default
                pass

    def default_config(self):
        """handy shortcut to detect commonly used config paths"""
        return [os.path.join(os.environ.get('XDG_CONFIG_HOME', '~/.config/'), self.prog + '.yml')]

I simply use it like this:

parser = ConfigArgumentParser()
parser.add_argument('--config', action=YamlConfigAction, default=ConfigArgumentParser.default_config())

This will load the config from ~/.config/argv0.yml as default values for other parameters on the commandline, or from whatever config file specified by the --config argument. It is overridable on the commandline and everything, but doesn't write its own config file either.

Anyways, I hope that helps... The TL;DR: of all the above is it's easy to preset defaults from a config file for simple parsers (parser.set_defaults() with a dict) but it becomes possibly more complicated with sub-parsers, in which case you might want to have add more tweaks to the above...

@ronhanson

This comment has been minimized.

Copy link

ronhanson commented Jan 31, 2019

If you are having issues with Configargparse, or new feature requests, feel free to fill bug reports and do PR. I will try my best to move the project forward and fix the existing bugs.

@anarcat

This comment has been minimized.

Copy link

anarcat commented Jan 31, 2019

@ronhanson there are many issues in certbot that are ultimately upstream issues in configargparse. for example, bw2/ConfigArgParse#104 and #6049 are two issues here specific to the project...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment