Skip to content
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

ArgumentParser as a container #227

Closed
skrobinson opened this issue Oct 31, 2022 · 6 comments · Fixed by #245
Closed

ArgumentParser as a container #227

skrobinson opened this issue Oct 31, 2022 · 6 comments · Fixed by #245

Comments

@skrobinson
Copy link
Contributor

Recent Issues (#217, #224) make me think people want container-like access to ArgumentParser. I'm considering adding ArgumentParser::at to return Argument and subparser instances.

template <typename T = Argument>
T ArgumentParser::at(const std::string key) {}

auto help_arg = program.at("--help");
auto add_cmd = program.at<ArgumentParser>("add");

Thoughts, @p-ranav, @lingerer, @bwpge?

@bwpge
Copy link

bwpge commented Oct 31, 2022

@skrobinson hmm good question... I'm definitely not against having container-like access like you have here.

I think maybe there's an alternate route to go with inheritance and making the members protected. I'm not sure if maybe this would make testing insanely more difficult, but in the spirit of Python's argparse this is typically what you'll find recommended when the standard one doesn't meet requirements.

@p-ranav
Copy link
Owner

p-ranav commented Oct 31, 2022

I'm not a fan of leaking the core of the internal implementation details and would prefer to avoid it. Future versions of argparse may favor a different data structure to do something. This has already happened a few times in the past where the data structures have been changed either for performance or for convenience. If we expose the internals, then further changes become either difficult or impossible to make while also keeping backwards compatibility.

From any research I've done on this, it appears that people create some kind of BaseCommand class with a handle member function and define derived classes that implement this handle member function. These handle member function accept an argparse.Namespace object that provides the context.

I haven't found any example where a developer directly inherits from argparse.ArgumentParser, you can correct me on this.

I'm certainly a fan of ArgumentParser::at working for both arguments and parsers. It's an easier change to make and would work fine.

@lingerer
Copy link

lingerer commented Nov 1, 2022

I felt a little bit confuse about it. What's the main difference vs [] in ArgumentParse? [] looks like also provide the reference of Argument.

@p-ranav
Copy link
Owner

p-ranav commented Nov 1, 2022

With .at<T>() you can specify the type and get a reference to not only an Argument but also an ArgumentParser (which happens to be a subcommand).

@skrobinson
Copy link
Contributor Author

@lingerer The difference is that I misread operator[] ages ago and thought it was just an alternate syntax for ArgumentParser::get(). Your question prompted me to look again. Thank you.

But, the main advantage I see to .at<T>() is that operator templates are verbose to use.

program.at("--help")
program.at<ArgumentParser>("add")

// versus

program["--help"]
program.operator[]<ArgumentParser>("add")

@lingerer
Copy link

lingerer commented Nov 2, 2022

If it provide more type transform or something for the reference , I think it will be a better way than [] just return as it is.

skrobinson added a commit to skrobinson/argparse that referenced this issue Nov 10, 2022
This allows updating attached object properties without holding external
references to the various Argument and ArgumentParser objects.

Closes p-ranav#227

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
skrobinson added a commit to skrobinson/argparse that referenced this issue Nov 29, 2022
This allows updating attached object properties without holding external
references to the various Argument and ArgumentParser objects.

Closes p-ranav#227

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment