typed-argparse / typed-argparse

💡 write type-safe and elegant CLIs with a clear separation of concerns.
https://typed-argparse.github.io/typed-argparse/
MIT License
28 stars 6 forks source link

add support for setting metavar #68

Closed MikkelSchubert closed 1 year ago

MikkelSchubert commented 1 year ago

This patch exposes the metavar option and allows you to use more descriptive placeholders in help text. This is especially useful for long command line option, where the default metavars are equally long:

database_old: Path = tap.arg(help="some help")
database_new: Path = tap.arg(help="some help", metavar="FILE")

becomes

options:
  -h, --help            show this help message and exit
  --database-old DATABASE_OLD
                        some help
  --database-new FILE   some help

I considered if it was possible to prevent using metavar for bool arguments using the type system, but it only seems possible when using default/dynamic_default and not (what I assume is) the common case where you don't use either. Misusing metavar also doesn't violate type safety, so it didn't seem worthwile to expand the overloads just for that.

codecov-commenter commented 1 year ago

Codecov Report

Merging #68 (73c28c4) into master (cef3707) will increase coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #68      +/-   ##
==========================================
+ Coverage   92.89%   92.93%   +0.04%     
==========================================
  Files           9        9              
  Lines         704      708       +4     
==========================================
+ Hits          654      658       +4     
  Misses         50       50              
Files Changed Coverage Δ
typed_argparse/arg.py 77.14% <100.00%> (+0.21%) :arrow_up:
typed_argparse/parser.py 95.41% <100.00%> (+0.05%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

bluenote10 commented 1 year ago

Thanks for the PR! Yes I'd say exposing the metavar should be fine.

I considered if it was possible to prevent using metavar for bool arguments using the type system, but it only seems possible when using default/dynamic_default and not (what I assume is) the common case where you don't use either. Misusing metavar also doesn't violate type safety, so it didn't seem worthwile to expand the overloads just for that.

Yes that should be fine. I'm actually contemplating to split the tap.arg into multiple functions so that differences in their interfaces don't have to be modeled entirely via overloads. I haven't thought about how the split will look like exactly (perhaps positional vs non-positional, scalar vs variadic/nargs) but if we'd have a separate group for boolean switches, we could omit metavar there, making it fully type safe.

Looking at argparse's metavar docs (last paragraph) it seems that it even allows for Optional[str | tuple[str, ...]], but that can still be added later if needed.

MikkelSchubert commented 1 year ago

Looking at argparse's metavar docs (last paragraph) it seems that it even allows for Optional[str | tuple[str, ...]], but that can still be added later if needed.

I have to admit that I forgot about that, but I wouldn't mind drafting a patch for that if you'd like to support that as well.

bluenote10 commented 1 year ago

I have to admit that I forgot about that, but I wouldn't mind drafting a patch for that if you'd like to support that as well.

Not super crucial. It actually raises the question if this is supported consistently across all supported argparse / Python versions. What if e.g. nargs=3 but the tuple only has a length of 2 etc? Will it generate ... or other repetition markers for nargs="*" implicitly? For now just supporting str may suffice.