python / cpython

The Python programming language
https://www.python.org
Other
63.35k stars 30.34k forks source link

Have argparse provide ability to require a fallback value be present #70582

Open c3fd86a6-5256-4135-84fa-e8b9d6e2463c opened 8 years ago

c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago
BPO 26394
Files
  • fallback.py: use case
  • argparse-wip-1.patch
  • example.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'library'] title = 'Have argparse provide ability to require a fallback value be present' updated_at = user = 'https://bugs.python.org/quabla' ``` bugs.python.org fields: ```python activity = actor = 'brett.cannon' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'quabla' dependencies = [] files = ['41984', '42061', '42062'] hgrepos = [] issue_num = 26394 keywords = ['patch'] message_count = 11.0 messages = ['260568', '260572', '260611', '261123', '261537', '261540', '261541', '261568', '261585', '261599', '294665'] nosy_count = 3.0 nosy_names = ['bethard', 'paul.j3', 'quabla'] pr_nums = [] priority = 'normal' resolution = None stage = 'test needed' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue26394' versions = ['Python 3.5'] ```

    c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago

    argparse has at least three features to set defaults (default=, set_defaults(), argument_default=). However, there is no feature to set the values of arguments.

    The difference is, that a required argument has to be specified, also if a default value exists. Thus, a clean way to set values from config files or env variables does not exist without making all arguments optional.

    See for example \http://stackoverflow.com/questions/10551117/setting-options-from-environment-variables-when-using-argparse\, where it becomes clear that no general solution for all actions exists. As a result, people also start to mess around with the args parameter of parse_args(). Even ConfigArgParse (used by Let's Encrypt) seems to fail in doing this properly.

    So please add a set_values() function, similar to set_defaults(). The predefined value should be treated as if it had been entered on the command line. If the value has also been supplied via the command line, the predefined value is overwritten.

    brettcannon commented 8 years ago

    To paraphrase Michael, he wants a way to tell argparse that an argument has to be supplied either from the command-line or some other mechanism (e.g., envvar, config file, etc.), but that if the value cannot be found in either situation, argparse fails saying the value is missing. This is different from a default value and making something optional as argparse has no way to signal that a default value isn't available, forcing the user to do the actual check externally to argparse itself. All of this stems from the fact that argparse's default argument stuff happens prior to parsing sys.argv and determining what is missing.

    What this probably requires is a new keyword-only argument like fallback which is a callable which is only called if an accompanying value isn't found from sys.argv and which can raise some specific exception to signal that the fallback value couldn't be determined on top of missing from sys.argv.

    c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago

    Thank you for clarifying my request. Callables sound like a great solution to me!

    Looks like the fallback should be called in take_action() if argument_values is "empty" (None?). Per default it should be transparent to an Action if the values are based on a fallback call. However, it might be useful to call Action with two additional parameters like commandline_values and fallback_values such that a (custom) action 'append_with_fallback' can be realized. Calling the fallback each time (required for fallback_values) makes it some kind of early called hook for argparse.

    I am attaching a sketch of what might be a typical use case. I noted that a callable supplied to fallback would usually rely on informations that could be provided as action.long_option_names and action.is_positional to avoid that this logic is implemented outside of argparse again.

    Please let me know I can provide any help.

    c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago

    I have prepared a working patch to sketch how this could be implemented. A small example (example.py) shows how this feature can be used to implement a fallback to environment variables.

    This patch allows Currying of positional arguments (i.e. you can give position 2 via fallback and provide position 1 via the command line). However, I think this might be too confusing and implicit.

    Please let me know whether I should prepare a proper patch.

    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

    I need to study that patch more, but I don't like the fact that it operates at the core of the parsing (e.g. in 'take_action'). That's a dangerous place to add features. Unexpected behaviors and backwards compatibility problems are too likely.

    It would be better if the enhancement could be made to things that are already being subclassed, like Actions. But I'll have to think more about alternatives.

    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

    Let's see if I understand the proposed patch

    Each call to 'take_action' is replaced with a save to a 'action_with_values' dictionary

    For a flagged (optional) it saves the 'args' and 'option_string'; for positionals it save 'args':

    Then at the end of the 'consume_optional' and 'consume_positionals' loops, it processes all the deferred 'take_actions'

    + for action, kwargs in actions_with_values.items(): + take_action(action, **kwargs)

    It had previously looped through 'parser._actions' and created an entries in 'actions_with_values' with actions and 'fallback_strings' key.

    In 'take_action', if the action's 'argument_strings' key (in actions_with_values) is None, it uses the 'fallback_strings' instead.

    ------------------

    Has this patch been tested? That is, does it pass all the tests in 'test_argparse.py'?

    One potential failing is that the order in which 'take_action' occurs can change. In the original, the order of 'take_action' depends on the order of occurrence in the sys.argv list. With this change, the order depends on the hashing order in 'actions_with_values'.

    In many cases that order does not matter. But there's nothing, in the current code, to prevent order dependence. Even if the user does not define custom Actions, actions like 'append' are execution order dependent. And optionals may occur several times, e.g

    python prog.py -f one --foo two -f three

    the namespace could have (foo='three') or (foo=['one','two','three']) depending the Action class.

    And different arguments can save to the same 'dest'.

    Deferring 'take_action' like this is too fraught with backward compatibility issues to consider seriously. Parsing is complex enough as it is, without adding the uncertainty of this differed 'take_action'.

    Other qualms:

    --------------

    An alternative place to apply this kind of 'fallback' is at the end of '_parse_known_args'. At this point the parser has access to 'see_actions' and 'seen_non_default_actions' lists (or sets). It uses those to test for 'required_actions' and required mutually_exclusive_groups.

    In http://bugs.python.org/issue11588 (Add "necessarily inclusive" groups to argparse) I propose adding a 'hook' at this point that can be used apply more general group tests (not just the current xor, but all the other logical possibilities). This hook could look at which actions were seen (i.e. acted on by take_action), and raise errors if the wrong combination was seen or not seen. The idea is that access to 'see_actions' is more definitive than checking the namespace of 'is None' values.

    I can imagine a fallback operating as part of this hook, filling in value for required actions that were not seen.

    This is similar to looking at and modifying the args namespace after parsing, except that it has access to the 'seenactions' set. By acting at this point, the fallback is not constrained by the action's \_call__ or any of its parameters (nargs, type, etc). There are pros and cons to that.

    -----------------------

    I've mentioned else where (including SO questions) that 'ipython' uses argparse along with config. It does so by first reading config, and then populating the parser with arguments derived from the config. That way the user has several ways of setting values - the default config, profile configs, and the commandline. In practice most values come from config, and only a select few get changed via commandline.

    argparse already as a poor-man's config file mechanism, the 'fromfile_prefix_chars'. This reads strings from a file and splices them into the 'argv' list.

           if self.fromfile_prefix_chars is not None:
                arg_strings = self._read_args_from_files(arg_strings)

    It's not as powerful as config or reading the environment, but it still provides a way of adding strings to those already given in the commandline.

    In short, I think it best to exhaustively look at alternatives that don't require surgery to the heart of parse_args. A feature like this can only be added if it has absolutely zero chance of modifying the behavior of anyone else's parser.

    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

    http://stackoverflow.com/a/35144270/901925 dynamically-set-default-value-from-cfg-file-through-argparse-python

    is my answer to a similar recent (Feb 2015) SO question.

    I rather like the idea of using collections.ChainMap on the dictionary versions of the namespace and config data.

    c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago

    Thanks so much for looking into my patch!

    Let me start by outlining that I don't understand how your alternate solutions are coping with the key problem. You are assuming that you get a populated Namespace object. However, _parse_known_args is exiting if required arguments are missing. Hence, late hooks or working with Namespaces is just too late. I was thinking about adding an option that suppresses required argument errors, but this again implies that parts of the logic have to be reimplemented outside of argparse.

    Let me remind you of ConfigArgParse again. It constructs an argument string from configs etc. and appends it to sys.argv before calling argparse. Surprise: This is not rock solid and limits the capabilities. There is almost no other way to attach these functionalities to argparse.

    ------------------------------------------------------------------------

    The actions_with_values dict should actually be ordered. My bad. With an ordered dict, the order can only be changed if a fallback is present. The desired behaviour has to be defined in this context. However, without fallbacks the order is identical with an ordered dict. I totally get that backward compatibility is a delicate thing in this context.

    Unit tests: All passing, up to action='append'. This is related to a design decision to be made for fallbacks in case of multiply supplied arguments.

    Number of arguments for fallbacks: This part was blind guessing and it worked for my simple test cases. I really didn't dive into this regex part in detail. At least extensive unit tests for fallbacks are required anyways.

    Values and objects instead of strings: Think there are two aspects.

    1. That example.py had to handle bools explicitly is a design decision. How bools can be supplied in configs or env vars should not be hard coded in argparse. This also applies for my [x,y] list syntax etc.

    2. Supplying actual objects/values as fallback makes sense. The current implementation (with my example) is such, that you can supply the arguments in the same form as on the command line. If you have interpreted this information already (say, fallback returns true instead of []), you don't want the returned fallback to be handled in the usual way [number of arguments differs (it's not even a list), action gets unexpected input]. Maybe the fallback can indicate that by returning an argparse.FinalValue() object or what ever.

    Subparsers: Yeah, big unknown. But my intuition is, that this can be solved nicely.

    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 8 years ago

    Yes, the use of ordered dictionary would be right. Your code would still reorder actions that have fallback values, but that wouldn't affect others.

    Yes, a fallback hook at the end of parsing would have to be placed before the 'required' testing, not after as in my 'group' testing.

    I still haven't absorbed why this patch is superior to the ConfigArgParse approach, or chaining dictionary values after parsing.

    By the way I like how ConfigArgParse claims to automatically test itself against 'test_argparse.py'. I'd also suggest following its example in published this enhancement in your own repository and pypi. The process of adding features to the main Python distribution is very slow.

    c3fd86a6-5256-4135-84fa-e8b9d6e2463c commented 8 years ago

    Yes, a fallback hook at the end of parsing would have to be placed > before the 'required' testing, not after as in my 'group' testing.

    Just to clarify: Would you suggest that a fallback call returns the parsed value (i.e. the fallback calls the action or alike) or take_action is called on fallback values after all the other calls are done?

    I will try to look into your hook system in detail. Maybe you are right and the ideas can be combined nicely.

    I still haven't absorbed why this patch is superior to the ConfigArgParse approach, or chaining dictionary values after parsing.

    Problems with ConfigArgParse I found: It ignores config and env vars completely if "--" is used on the command line. Implementing positional arguments supplied via config or env var is impossible.

    Chaining dictionary would require that all arguments are optional. I think that the "make everything optional" approach bypasses to many features of argparse.

    I'd also suggest following its example in published this enhancement in your own repository and pypi.

    Thanks for the suggestion. I will surely consider this.

    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 7 years ago

    http://bugs.python.org/issue29670 argparse: does not respect required args pre-populated into namespace

    wants to put 'required' values in the Namespace. That's a similar issue.

    An idea that I tried for that (but didn't submit as a patch) is to move the tests at the end of _parse_know_args to a separate function that is called by parse_known_args.

    In http://bugs.python.org/issue26394#msg261540 I suggested putting the fallback in the testing section of _parse_known_args. If that testing section is in a separate method, it would be easier to customize and add such a fallback.