Add return_unknown_args to tyro.cli

[JesseFarebro]

As per #34.

[JesseFarebro]

Is there a reason you were casting the return from _cli_impl instead of just typing it via overload?

[brentyi]

That was fast, thanks!

One thing: could you check the logic in def fix_args()? I'm worried this could confusingly replace underscores with hyphens in the returned arguments.

On using overloads for _cli_impl(): no particular reason, I think in this case using an overload just had no extra type safety benefits, and it's a bit annoying to repeat the signature.

[codecov]

Codecov Report

Base: 99.19% // Head: 99.20% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (fdf6663) compared to base (1917cc4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #35   +/-   ##
=======================================
  Coverage   99.19%   99.20%           
=======================================
  Files          21       21           
  Lines        1624     1642   +18     
=======================================
+ Hits         1611     1629   +18     
  Misses         13       13           

Flag	Coverage Δ
unittests	99.20% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tyro/_cli.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[JesseFarebro]

Could there be some compromise on this fix args feature? This will be a problem.

What I'm thinking is the following: Instead of fixing the arguments like this could we instead have a kwarg where option strings would either have:

1. All dashes
2. All underscores
3. Both dashes and underscores (use multiple option strings).

It's not really feasible to add option strings for every possible combination of dash and underscore for each part of the prefix so a compromise that's all or none or both might be fine.

I already have this implemented by passing around a tuple of strings for the field paths instead of converting the field path to a string at each step of the recursion. This just gives more flexibility w.r.t. option strings when lowering the argument definition.

If you're okay with this change I can either make a different PR or add it to this PR.

[brentyi]

Thanks for looking into that!

On adding a kwarg: I'd prefer to be conservative about adding things to the public API; my feeling is this hyphen vs underscore preference isn't something that enough people will care enough to justify adding.

It won't be pretty, but for the sake of this PR maybe we can just track a fixed arg -> original arg map, which can be used to "unfix" any extra args returned by parse_known_args()? Would that work?

Either way the tuple change sounds like it could be useful if we want to add support for aliases, which I know people have expressed interest in.

--

Aside: probably we need a better name than "fix" for the hyphen / underscore conversion here, I keep getting it confused with the concept of "fixed args" as in tyro.conf.Fixed, which are arguments that can't be changed via the CLI.

[JesseFarebro]

Good call on the kwarg, I'd generally agree with this.

I implemented a simple mapping as you suggested. In the case where two options are ambiguous, e.g., --a_b and --a-b I raise a RuntimeError. Do you want a custom exception type here, something like AmbigousOptionError?

[JesseFarebro]

Can I get you to approve the CI workflows again? If they all pass we should be in good shape.

[brentyi]

This looks great; just a few last things, thanks for being so responsive!!

[JesseFarebro]

Rebased on main with #37.

[brentyi]

Thanks @JesseFarebro! Appreciate your help on this.