Skip to content

Commit

Permalink
Fix positional arg corner case, help formatting for tyro.conf.arg()
Browse files Browse the repository at this point in the history
  • Loading branch information
brentyi committed Nov 12, 2022
1 parent 7b3a70a commit 77770cc
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 17 deletions.
17 changes: 14 additions & 3 deletions examples/04_additional/06_conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
Usage:
`python ./06_conf.py --help`
`python ./06_conf.py 5 --boolean True`
"""

import dataclasses
Expand Down Expand Up @@ -34,15 +35,25 @@ class CommitArgs:

@dataclasses.dataclass
class Args:
# A boolean field with flag conversion turned off.
boolean: tyro.conf.FlagConversionOff[bool] = False

# A numeric field parsed as a positional argument.
positional: tyro.conf.Positional[int] = 3

# A boolean field with flag conversion turned off.
boolean: tyro.conf.FlagConversionOff[bool] = False

# A numeric field that can't be changed via the CLI.
fixed: tyro.conf.Fixed[int] = 5

# A field with manually overridden properties.
manual: Annotated[
str,
tyro.conf.arg(
name="renamed",
metavar="STRING",
help="A field with manually overridden properties!",
),
] = "Hello"

# A union over nested structures, but without subcommand generation. When a default
# is provided, the type is simply fixed to that default.
union_without_subcommand: tyro.conf.AvoidSubcommands[
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "tyro"
version = "0.3.32"
version = "0.3.33"
description = "Strongly typed, zero-effort CLI interfaces"
authors = ["brentyi <[email protected]>"]
include = ["./tyro/**/*"]
Expand Down
16 changes: 16 additions & 0 deletions tests/test_conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -527,3 +527,19 @@ def main(x: Any = Struct()) -> int:

assert tyro.cli(main, args=[]) == 5
assert tyro.cli(main, args=["--x.nice", "3"]) == 3


def test_positional():
def main(x: tyro.conf.Positional[int], y: int) -> int:
return x + y

assert tyro.cli(main, args="5 --y 3".split(" ")) == 8
assert tyro.cli(main, args="--y 3 5".split(" ")) == 8


def test_positional_order_swap():
def main(x: int, y: tyro.conf.Positional[int]) -> int:
return x + y

assert tyro.cli(main, args="5 --x 3".split(" ")) == 8
assert tyro.cli(main, args="--x 3 5".split(" ")) == 8
17 changes: 6 additions & 11 deletions tyro/_arguments.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,8 @@ def add_argument(

# Apply overrides in our arg configuration object.
# Note that the `name` field is applied when the field object is instantiated!
kwargs.update(
{
k: v
for k, v in vars(self.field.argconf).items()
if v is not None and k != "name"
}
)
if self.field.argconf.metavar is not None:
kwargs["metavar"] = self.field.argconf.metavar

# Add argument! Note that the name must be passed in as a position argument.
arg = parser.add_argument(name_or_flag, **kwargs)
Expand Down Expand Up @@ -314,13 +309,13 @@ def _rule_generate_helptext(

help_parts = []

docstring_help = arg.field.helptext
primary_help = arg.field.helptext

if docstring_help is not None and docstring_help != "":
if primary_help is not None and primary_help != "":
# Note that the percent symbol needs some extra handling in argparse.
# https://stackoverflow.com/questions/21168120/python-argparse-errors-with-in-help-string
docstring_help = docstring_help.replace("%", "%%")
help_parts.append(_rich_tag_if_enabled(docstring_help, "helptext"))
primary_help = primary_help.replace("%", "%%")
help_parts.append(_rich_tag_if_enabled(primary_help, "helptext"))

default = lowered.default
if lowered.is_fixed():
Expand Down
2 changes: 1 addition & 1 deletion tyro/_calling.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ def get_value_from_arg(prefixed_field_name: str) -> Any:
consumed_keywords |= consumed_keywords_child

if value is not _fields.EXCLUDE_FROM_CALL:
if field.is_positional():
if field.is_positional_call():
args.append(value)
else:
kwargs[field.call_argname] = value
Expand Down
13 changes: 12 additions & 1 deletion tyro/_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def make(
else:
assert len(argconfs) == 1
(argconf,) = argconfs
helptext = argconf.help

typ, inferred_markers = _resolver.unwrap_annotated(typ, _markers.Marker)
return FieldDefinition(
Expand All @@ -102,13 +103,23 @@ def add_markers(self, markers: Tuple[_markers.Marker, ...]) -> FieldDefinition:
)

def is_positional(self) -> bool:
"""Returns True if the argument should be positional in the commandline."""
return (
# Explicit positionals.
_markers.Positional in self.markers
# Dummy dataclasses should have a single positional field.
or self.name == _strings.dummy_field_name
)

def is_positional_call(self) -> bool:
"""Returns True if the argument should be positional in underlying Python call."""
return (
# Explicit positionals.
_markers._PositionalCall in self.markers
# Dummy dataclasses should have a single positional field.
or self.name == _strings.dummy_field_name
)


class PropagatingMissingType(_singleton.Singleton):
pass
Expand Down Expand Up @@ -646,7 +657,7 @@ def _field_list_from_params(
typ=hints[param.name],
default=default,
helptext=helptext,
markers=(_markers.Positional,)
markers=(_markers.Positional, _markers._PositionalCall)
if param.kind is inspect.Parameter.POSITIONAL_ONLY
else (),
)
Expand Down
4 changes: 4 additions & 0 deletions tyro/conf/_markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
"""A type `T` can be annotated as `Positional[T]` if we want to parse it as a positional
argument."""

# Private marker. For when an argument is not only positional in the CLI, but also in
# the callable.
_PositionalCall = Annotated[T, None]

# TODO: the verb tenses here are inconsistent, naming could be revisited.
# Perhaps Suppress should be Suppressed? But SuppressedFixed would be weird.

Expand Down

0 comments on commit 77770cc

Please sign in to comment.