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

Overhaul ArgParse to make it more like python argparse #2531

Merged
merged 3 commits into from Mar 31, 2020

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Mar 27, 2020

I've been thinking about overhauling ArgParse for a couple years, and
did most of the work in December when I was home from work. Then sat
on it except for slow burn weekend tinkering, but finally got a chance
to put the final touches on and write the docs this week.

The bottom line is that this is a major rewrite of the APIs for
ArgParse to make them much richer, more flexible, and more closely
resembling those of python's argparse module.

Rather than trying to explain the syntax here, I simply will refer you
to the extensive inline documentation in argparse.h, as well as the
examples in the code base of where it's used.

It sill respects the old syntax, so this change is hoped to not alter
the behavior of any programs using the deprecated API.

Start by reading argparse.h, it's design review of the API that is the
most important for those inclined to weigh in.

@fpsunflower
Copy link
Contributor

At first glance this seems like a nice change.

I prefer the style that binds the option to a specific variable directly, rather querying it in a second place with ap.get() (which means you have to type the option name again). The programs you updated seem to use both styles (sometimes within one program), but perhaps this was intentional?

Overall LGTM!

@lgritz
Copy link
Collaborator Author

lgritz commented Mar 28, 2020

Right, I'm using a mix of styles currently. This is some combination of (a) I'm not sure what I prefer myself yet! (b) Laziness, I did kind of a more straightforward conversion to an intermediate step I had devised, then later worked my way to the full argparse implementation and only in a few cases did I return and fix all the uses.

At some point, the conversion of all the uses was grueling and I just wanted to get a draft posted and merged. I will probably continue to change how I use it in different places, but I just wanted to finally push this out after months of tinkering.

I think there are three different "styles", each of which I prefer in different situations:

  1. Full emulation of python argparse, as close as we can get in C++. Maybe people who spend a lot of time in python or who come to the table familiar with or liking python argparse will like this best.

    ap.arg("-color")
        .nargs(3)
        .metavar("R G B");
    
    Color3f c = ap["color"].get<Color3f>();
    
  2. More compact notation, same retrieval, but uses a different notation than python argparse. I think this is my current favourite, especially for the parameters you just use once.

    ap.arg("-color R G B");
    Color3f c = ap["color"].get<Color3f>();
    
  3. External storage of results. I think this is ugly and error prone, but sometimes it really is the best choice if you're just going to pull out those values and store them in the variables anyway.

    Color3f c;
    ap.arg("-color %f:R %f:G %f:B", &c[0], &c[1], &c[2]);
    

I've been thinking about overhauling ArgParse for a couple years, and
did most of the work in December when I was home from work. Then sat
on it except for slow burn weekend tinkering, but finally got a chance
to put the final touches on and write the docs this week.

The bottom line is that this is a major rewrite of the APIs for
ArgParse to make them much richer, more flexible, and more closely
resembling those of python's argparse module.

Rather than trying to explain the syntax here, I simply will refer you
to the extensive inline documentation in argparse.h, as well as the
examples in the code base of where it's used.

It sill respects the old syntax, so this change is hoped to not alter
the behavior of any programs using the deprecated API.

Start by reading argparse.h, it's design review of the API that is the
most important for those inclined to weigh in.
@fpsunflower
Copy link
Contributor

That maybe brings up one thing:

If I'm reading the code correctly, you have to say:

int foo;
std::string bar;
ap.arg("-foo %d", &foo);
ap.arg("-bar %s", &bar);

Borrowing the %d and %s makes sense, but in theory the type information is more "robustly" available from the pointer argument. Without looking at the implementation details at all -- what do you think of attempting something akin to the printf vs fmt libraries so that you could do:

int foo;
std::string bar;
Color3f baz;
ap.arg("-foo {}", &foo);
ap.arg("-bar {}", &bar);
ap.arg("-baz {}", &baz);

I don't want to derail the review - this already looks like a nice set of changes, just something I had wondered about the old API as well that seems to maybe still be relevant to this new iteration.

@lgritz
Copy link
Collaborator Author

lgritz commented Mar 28, 2020

Yeah, it would be nice to eliminate those cues, just say:

ap.arg("-foo X Y", &xvar, &yvar);

and it infers the types from the pointers.

Let's say, I'll keep this in mind as a later step.

The reason they didn't come along yet is:

  1. Inertia, I guess?

  2. Doing this right is a bit of painful template-fu that will require a bit of head scratching to get right.

  3. It's hard to imagine it working quite like what you said above, how would it know exactly that a Color3 requires 3 floats? I mean, we could hard code Color3, Vec3, whatever, but that seems a little inelegant, too.

But anyway, yeah, this is not the final chapter on ArgParse. I'm sure lots of refinement will come later.

@lgritz lgritz merged commit e81036b into AcademySoftwareFoundation:master Mar 31, 2020
@lgritz lgritz deleted the lg-argparse branch March 31, 2020 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants