tyro icon indicating copy to clipboard operation
tyro copied to clipboard

Feature: Markers: SelectionFromEnumValues

Open emcd opened this issue 1 year ago • 4 comments

  • Add configuration marker: SelectionFromEnumValues

  • Refactor instantiator production logic for types with automatically-populated choices. Encapsulation to keep related variables tracked togther.

Resolves #166 .

@brentyi : All linter checks and tests are passing. Not sure if you want anything additional for documentation; looks like you have an automatic generator which I did not look closely at.

Also, manual proof that it works as described and intended:

>>> import dataclasses, enum, typing_extensions, tyro
>>> class Formats( enum.StrEnum ):
...     JSON = enum.auto( )
...     PRETTY = enum.auto( )
...
>>> @dataclasses.dataclass
... class Args:
...     format: typing_extensions.Annotated[ Formats, tyro.conf.SelectFromEnumValues ] = Formats.PRETTY
...
>>> tyro.cli( Args, args = [ '--format', 'json' ] )
Args(format=<Formats.JSON: 'json'>)
>>> tyro.cli( Args, args = [ '--format', 'pretty' ] )
Args(format=<Formats.PRETTY: 'pretty'>)
>>> tyro.cli( Args, args = [ ] )
Args(format=<Formats.PRETTY: 'pretty'>)
>>> tyro.cli( Args, args = [ '--help' ] )
usage: [-h] [--format {json,pretty}]

╭─ options ───────────────────────────────────────────────╮
│ -h, --help              show this help message and exit │
│ --format {json,pretty}  (default: pretty)               │
╰─────────────────────────────────────────────────────────╯

emcd avatar Sep 22 '24 21:09 emcd

Code coverage error appears to be a Codecov rate-limiting issue.

However, I just realized that I missed the Literal case that you mentioned in the issue. I need to cover that and make tests accordingly. So, please do not merge yet.

emcd avatar Sep 22 '24 22:09 emcd

@brentyi : I just added the enum values case for literal lists that I forgot earlier. Should be good to go.

emcd avatar Sep 22 '24 23:09 emcd

Thanks @emcd for the PR!! Just wanted to note that reviewing/merging this is on my to-do list, unfortunately this week has been busier than most...

brentyi avatar Sep 25 '24 22:09 brentyi

No worries, @brentyi . Life (Ph.D. theses, etc...) comes first. I've been using a fork of your repo with my patch, so I'm not blocked.

I am going to file another feature request soon, though, but no pressure in responding to it.

emcd avatar Oct 05 '24 16:10 emcd

Thanks for understanding @emcd! I just pushed some minor changes to your branch, could you confirm that they look reasonable?

  • You had defined some classes for handling the different ways of generating choices. I refactored a bit to make it more succinct / self-contained. It might be less type-safe.
  • I relaxed the constraint that values need to be strings + added a test.

brentyi avatar Oct 09 '24 22:10 brentyi

The changes seem good to me, @brentyi . I actually introduced the classes to make things a bit more self-contained so that we weren't working with two or three different related variables, side by side, in multiple places. But, I have no objection to what you have done.

I just added a commit to fix the example to match the renamed marker.

emcd avatar Oct 10 '24 00:10 emcd

Thanks for the fix! And the help with this feature. For the classes your explanation makes sense, seems like just a matter of personal taste. October is feeling like a "limit abstractions" month for me 😛

brentyi avatar Oct 10 '24 20:10 brentyi