python / cpython

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

Add ability to retrieve choices from `argparse.ArgumentParser` #125363

Open cohml opened 1 month ago

cohml commented 1 month ago

Feature or enhancement

Proposal:

argparse.ArgumentParser provides a get_default method (source) for retrieving an argument's default value:

import argparse
parser = argparse.ArgumentParser()
parser.add_argument("foo", default="bar")
parser.get_default("foo")  # 'bar'

However, there is currently no way to analogously retrieve the set of choices users can choose from:

parser.add_argument("foo", choices=["bar", "baz"])

So I propose adding a get_choices method, following the structure of get_default, which provides this ability:

# argparse.py

...

class _ActionsContainer(object):

    ...

    def get_choices(self, dest):
        for action in self._actions:
            if action.dest == dest:
                return action.choices

This should yield the following behavior:

import argparse
parser = argparse.ArgumentParser()
parser.add_argument("foo", choices=["bar", "baz"])
parser.add_argument("ham")
parser.get_choices("foo")  # ['bar', 'baz']
parser.get_choices("ham")  # None

Note that get_default also returns None when called on an argument where no default has been set:

>>> import argparse
>>> parser = argparse.ArgumentParser()
>>> _ = parser.add_argument("foo")
>>> print(parser.get_default("foo"))
None

Here's a demonstration of this proposal in action by extending argparse.ArgumentParser:

>>> import argparse
>>> class ExtendedArgumentParser(argparse.ArgumentParser):
...     def get_choices(self, dest):
...         for action in self._actions:
...             if action.dest == dest:
...                 return action.choices
...
>>> parser = ExtendedArgumentParser()
>>> _ = parser.add_argument("foo", choices=["bar", "baz"])
>>> _ = parser.add_argument("ham")
>>> print(parser.get_choices("foo"))
['bar', 'baz']
>>> print(parser.get_choices("ham"))
None

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

sobolevn commented 1 month ago

@savannahostrowski would you be interested? :)

savannahostrowski commented 1 month ago

@sobolevn Thanks for the tag - I'll take a look :)

cohml commented 1 month ago

@sobolevn @savannahostrowski So sorry guys! My original intention - and very strong desire - was to submit my own PR for this. I forgot to self-assign it to myself, but my branch is already underway and the PR will be out shortly.

I hope I don't ruffle any feathers by submitting the PR 😅

savannahostrowski commented 1 month ago

@cohml Go for it! Feel free to tag me for review.

cohml commented 1 month ago

Oh wait, I guess I can't self-assign - TIL!

Please feel free to assign this issue to me if necessary, and thanks again for accommodating my wishes!

picnixz commented 1 month ago

(I haven't seen a lot of issues being assigned but I think it helps indicating that someone is taking care of the issue without having to go through the entire thread, so I'm doing it anyway!)

serhiy-storchaka commented 1 month ago

I do not think this should be added. I recently closed a similar issue.

argparse does not have other getters. If you need some value, you should save it before passing to add_argument(). Or save the resulting action. You can then just retrieve the choices attribute of that action.

get_default() is the only exception, because it is non-trivial and there are many ways to specify default values (the default argument, set_defaults(), argument_default). I think that there was also more use cases for it.

savannahostrowski commented 1 month ago

@serhiy-storchaka While I see your point about this pattern not being super prevalent in the module, I do potentially see an argument here for providing these types of convenience methods. From my perspective, this isn't a breaking change, and I've seen a couple of issues on the repo asking for similar functions...which indicates to me that the way to achieve this may be non-obvious to some developers or just something that could be simpler.

Perhaps the surface area of argparse warrants a larger discussion and more robust design proposal, but I'm not necessarily convinced that just because we don't have many of these types of functions today means having them is a bad idea.

picnixz commented 1 month ago

Or save the resulting action. You can then just retrieve the choices attribute of that action

I agree with that but I think we could simply expose a way to retrieve an action according to its name. I don't think we have a get_action() getter but I think such getter would help. I have a project where I subclassed ArgumentParser and allowed to retrieve actions by their name just to inspect their metadata.

However, I'm not sure we want to expose actions like that since it could expose quite a lot of implementation details which we do not necessarily want =/ But I'd prefer returning the action rather than having getters for returning whatever underlying action's data we want (e.g., choices, nargs, etc).

serhiy-storchaka commented 1 month ago

Other issue closed by me: #115150.

If we add such methods, we will need to add also get_nargs(), get_type(), get_const(), get_help(), get_metavar(), get_option_strings(), get_dest(), get_actions(), get_optionals(), get_positionals(), get_argument_groups(), get_mutually_exclusive_groups(), etc, etc. The API will grow and grow. And this is not just an implementation and maintaining cost. This adds a burden on users. The argparse API is already large, and it is difficult to distinguish essential parts which you should learn to use argparse from functions with unclear use case which you may never use.

So I am against this. There are more than a hundred of open issues for argparse, and most of them are more important than this one. Several people have already spent more time on this issue than it deserves, and will spend even more if this feature is implemented.

Let focus on fixing real bugs, improving documentation, and adding more useful features.

cohml commented 1 month ago

If I get a vote, my view is that from the developer's perspective, these values are part of the ArgumentParser instance's state. As such, ArgumentParser's object-oriented nature leaves me feeling the values should be directly accessible, either as attributes or via some kind of getter. That seems like a very natural thing to expect.

I am sympathetic to the argument that it's a slippery slope: We already have get_default, so if we add get_choices, why not also add get_nargs, get_type, etc. And perhaps we should! But more practically, perhaps if no one is asking for get_foo, maybe it's okay not to have it. But get_default already got the nod, and I personally feel get_choices would be equally useful.

To the earlier point about

If you need some value, you should save it before passing to add_argument().

this may be easier said than done, and could require nontrivial refactoring to make possible depending on how your modules are designed.

cohml commented 1 month ago

Alternatively, perhaps an altogether simpler approach that satisfies all perspectives is to have one generic getter method which takes both an argument name and also an "attribute" name. For example:

parser.add_argument("foo", choices=["bar", "baz"], default="bar")
parser.get("foo", "choices")  # ['bar', 'baz']
parser.get("foo", "default")  # 'bar'
parser.get("foo", "nargs")    # None

Obviously this proposal would require much more careful deliberation and design. It also extends far beyond the scope of the current issue. But it could also be a way to add significant functionality without a corresponding increase in the surface area.

Edit: Or even more simply, perhaps an attribute that maps to a dictionary from which all argument attributes could be retrieved? For example:

>>> parser.add_argument("foo", choices=["bar", "baz"])
>>> parser.add_argument("ham", default=42)
>>> parser.arguments
{
    "foo": {
        "choices": ["bar", "baz"],
        "default": None,
        ...
    },
    "foo": {
        "choices": None,
        "default": 42,
        ...
    }
}

Then retrieving any attribute from any argument would be trivial, requiring only that you traverse the dictionary.

savannahostrowski commented 1 month ago

@cohml Would you mind sharing how you're planning on using this? Or any other relevant details about how you think this would practically solve a problem you're running into? Given that there's some hesitancy here, that might be helpful.

@serhiy-storchaka, I see your point. I'd probably advocate for leaving this issue open for a while to see if others want to weigh in or have other practical use cases for which they are looking for better support.

cohml commented 1 month ago

@savannahostrowski Sure! My immediate use case, which caused me to search the docs and open this issue upon discovering it's currently impossible, is this:

I have a somewhat long list of choices. For brevity, let's just say ["a", "b", "c"]. I also want to add a new value like "all" which, if passed, sets choices to the rest of the values. In code:

parser.add_argument("foo", choices=["a", "b", "c", "all"], nargs="+")
parser.parse_args(["a"]).foo       # ['a']
parser.parse_args(["a", "b"]).foo  # ['a', 'b']
parser.parse_args(["all"]).foo     # ['a', 'b', 'c']

A proposal earlier in the thread would have me do something like this:

CHOICES = ["a", "b", "c"]
parser.add_argument("foo", choices=CHOICES + ["all"], nargs="+")
args = parser.parse_args()
if "all" in args.foo:
    args.foo = CHOICES

However, where my ArgumentParser is created (so, where CHOICES would have to be defined) is not where the args get parsed. So to have the list of choices be accessible in both places, I'd have to either

None of those solutions is desirable to me. This minor implementation detail of argparse should not dictate the design of my software, IMHO.

Instead, ideally I'd like to achieve the desired functionality this way:

parser.add_argument("foo", choices=["a", "b", "c", "all"], nargs="+")
args = parser.parse_args()
if "all" in args.foo:
    args.foo = [choice for choice in parser.get_choices("foo") if choice != "all"]

I admit this is somewhat of a niche use case. But overall, I feel the potential use cases/value proposition for get_choices or equivalent would be broadly the same as with get_default, which has been floating around for well over a decade.

cohml commented 2 weeks ago

@savannahostrowski Shall I go ahead and submit a PR? Not sure how long you'd like to wait for input but it seems to have dried up for now...

savannahostrowski commented 6 days ago

I think it's probably more sustainable for us to consider whether we want to add getters consistently across the module rather than handle this as a one-off. It's not an impossibility, but I'd rather approach this intentionally. Let's hold off on opening a PR for now.

cohml commented 6 days ago

Understood. If you ever green-light it, I'd love to volunteer.