python / cpython

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

argparse: parser aliases in subparsers stores alias in dest variable #80845

Open 025f190b-6e94-41f2-8e20-df8d604262d9 opened 5 years ago

025f190b-6e94-41f2-8e20-df8d604262d9 commented 5 years ago
BPO 36664
Files
  • sample.py: Sample code with subparser (also in comment)
  • 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 = ['3.7', 'type-feature', 'library'] title = 'argparse: parser aliases in subparsers stores alias in dest variable' updated_at = user = 'https://bugs.python.org/PeterMcEldowney' ``` bugs.python.org fields: ```python activity = actor = 'paul.j3' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'Peter McEldowney' dependencies = [] files = ['48275'] hgrepos = [] issue_num = 36664 keywords = [] message_count = 5.0 messages = ['340515', '340525', '340595', '355759', '355779'] nosy_count = 3.0 nosy_names = ['paul.j3', 'nemeskeyd', 'Peter McEldowney'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue36664' versions = ['Python 3.7'] ```

    025f190b-6e94-41f2-8e20-df8d604262d9 commented 5 years ago

    I noticed that I have to add a lot more code to handle contexts in subparsers that I was expecting would be necessary. This is something I feel should be handled by the argparse library. What are your thoughts on this?

    If you run the sample code with the commands below, you can see that although I would want them to do the same thing, I have to add more lines into my code to achieve this. This becomes cumbersome/annoying when dealing with subparser trees.

    python3 sample.py subsection python3 sample.py s

    Sample code (also attached):

    import argparse
    
    def get_args(args=None):
        parser = argparse.ArgumentParser()
        subparser = parser.add_subparsers(dest='context')
    
        sub = subparser.add_parser('subsection', aliases=['s', 'sub', 'subsect'])
    
        return parser.parse_args(args)
    
    def my_subsection_function(args):
        print('my subsection was called')
    
    def invalid_context(args):
        print('my functon was not called <sadface>')
    
    def main(args=get_args()):  
        return {
            'subsection': my_subsection_function
        }.get(args.context, invalid_context)(args)
    
    if __name__ == "__main__":
        main()
    7a064fe6-c535-4d80-a11f-a04ed39056c5 commented 5 years ago

    I added a print(args) to clarify what you are talking about:

    2148:~/mypy$ python3 bpo-36664.py subsection Namespace(context='subsection') my subsection was called 2148:~/mypy$ python3 bpo-36664.py s Namespace(context='s') my functon was not called \<sadface> 2148:~/mypy$ python3 bpo-36664.py sub Namespace(context='sub') my functon was not called \<sadface>

    The value of args.context depends on what alias was used, not the primary name of the subparser.

    The help lists all aliases

    2148:~/mypy$ python3 bpo-36664.py -h usage: bpo-36664.py [-h] {subsection,s,sub,subsect} ...

    The sub-parser doesn't actually have a name. In self._name_parser_map each alias is a key with a parser object value. Multiple keys for a single value. The only thing that distinguishes 'subsection' is that was the first key in that dictionary.

    In effect the subparser Action object does not maintain a mapping from the aliases to the 'subsection' name. I can imagine some ways of deducing that mapping, but it's not going to be a trivial task.

    Unless someone comes up with a clever patch, I think the best choice is for you maintain your own mapping. For example write a utility that takes a 'name' and alias list, calls

        sub = subparser.add_parser('subsection', aliases=['s', 'sub', 'subsect'])

    and saves some sort of mapping from the aliases to 'subsection'. Then use that later when you use args.context.

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

    https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.add_subparsers

    shows how to use set_defaults in a parser to set a 'func' attribute. That method could just as well be used to set the true 'name' or any other kind of attribute that is unique to that parser.

    I think we should leave the code as is, storing the aliases in the subparsers' 'dest'. It's been that way since aliases were added, and changing it runs the risk of creating backward compatibility issues.

    There are enough work arounds if you don't want the aliases.

    e7156765-cbc4-40de-a178-4c6a47cd8ca6 commented 4 years ago

    I ran into the same problem. I know of the set_defaults() method, in fact, that is what I have always been using. But why was dest added if not to provide a better solution?

    I agree that changing it would _perhaps_ break some code, so I agree that this issue cannot be "fixed" for Python 3.7 (and probably 3.8). But what about 3.9 or 4.0?

    BTW much higher profile backward-incompatible changes do happen in Python; take PEP-563 (https://www.python.org/dev/peps/pep-0563/), for instance.

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

    Just clarify how the code currently works. subparsers is a positional Action of subclass _SubParsersAction. It has a nargs='+...', requiring at least one string, and taking all remaining strings. Its call has the standard signature. So everything that's special about subparsers is embodied in this Action subclass.

        def __call__(self, parser, namespace, values, option_string=None):
            parser_name = values[0]
            arg_strings = values[1:]
    
            # set the parser name if requested
            if self.dest is not SUPPRESS:
                setattr(namespace, self.dest, parser_name)
        # select the parser
        try:
            parser = self._name_parser_map[parser_name]
        ...

    So the parser_name is first string, the actual alias that user provided. It is added to the namespace if a dest was provided (the default dest is SUPPRESS). That's all of the relevant code - the alias is simply added to to Namespace.

    As mentioned before parser_name is used find the actual sub parser, which is called with the remaining arg_strings.

    Earlier in the subclasses add_parser method, the 'name' and 'alias' list are used to populate the '_name_parser_map' mapping, and also create the metavar that's used in the help display. But neither is saved as an attribute.

    ---

    I still think 'set_defaults' is the cleanest way of saving a unique name for a sub parser.

        parser_foo.set_defaults(func=foo, name='foo')

    One further note - if you make subparsers required, you should also set a dest name:

        parser.add_subparsers(dest='cmd', required=True)

    otherwise the missing-subparsers error message will raise an error. It needs to identify the missing action in some way. Functionally, this might be the most important reason to set the 'dest'.

    rolf-d2i commented 2 weeks ago

    The normal way to treat aliases is to use preprocessor to remove the aliases for the original word hence football with alias soccer will be processed as football internally in the code. This should be fixed, here is a simple way to patch it in running code (don't use this in production unless you are ok with your code breaking without warning).

    argparse._SubParsersActionOld = argparse._SubParsersAction
    class subParserC(argparse._SubParsersAction):
    
     def __init__(...):
                ....copy original init code...
    
     def add_parser(self, name, **kwargs):
        if("aliases" in kwargs):
            self.aliases_map = kwargs["aliases"]
            self.aliases_2_name = name
            #print("aliases",self.aliases_map)
        return argparse._SubParsersActionOld.add_parser(self, name,**kwargs)
    
     def __call__(self, parser, namespace, values, option_string=None):   
        nvals = values
        if(hasattr(self,"aliases_map") and values[0] in self.aliases_map):
             if(values[0] in self.aliases_map):
                 nvals = [self.aliases_2_name] + values[1:]
    
        argparse._SubParsersActionOld.__call__(self, parser, namespace, nvals, option_string=option_string)
    
     argparse._SubParsersAction = subParserC

    Patches the original argparse with a preprocessor, but not very stable because the internals might change without warning, that being said if the parser had a preprocessor input option (or other option) this solution could easily be made stable.

    Can we get a fix for this issue? at the very least an option to use the expected behaviour for aliases as this will not break code.