From baa07d29e8fce77de5d0a8234072ce35c221be8c Mon Sep 17 00:00:00 2001 From: DanCardin Date: Fri, 7 Jun 2024 17:07:39 -0400 Subject: [PATCH] feat: Improve union error messages. --- CHANGELOG.md | 4 ++ pyproject.toml | 2 +- src/cappa/annotation.py | 51 +++++++++++++--------- src/cappa/command.py | 2 +- tests/arg/test_file_io.py | 6 +-- tests/arg/test_literal.py | 22 ++++++++++ tests/arg/test_literal_intermixed_union.py | 10 +++-- tests/arg/test_mapping_failure.py | 2 +- tests/arg/test_parse.py | 2 +- 9 files changed, 70 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd4296..666a232 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## 0.20 +### 0.20.1 + +- feat: Improve union error message. + ### 0.20.0 - feat: Implement exclusive argument groups. diff --git a/pyproject.toml b/pyproject.toml index f653865..9f7471a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "cappa" -version = "0.20.0" +version = "0.20.1" description = "Declarative CLI argument parser." repository = "https://github.com/dancardin/cappa" diff --git a/src/cappa/annotation.py b/src/cappa/annotation.py index ab7eb1f..590b243 100644 --- a/src/cappa/annotation.py +++ b/src/cappa/annotation.py @@ -7,7 +7,7 @@ from typing_inspect import get_origin, is_literal_type from cappa.file_io import FileMode -from cappa.typing import T, backend_type, is_none_type, is_subclass, is_union_type +from cappa.typing import T, is_none_type, is_subclass, is_union_type __all__ = [ "parse_value", @@ -57,7 +57,7 @@ def parse_value( return origin if is_none_type(origin): - return parse_none() + return parse_none if is_subclass(origin, list): return parse_list(*type_args) @@ -87,8 +87,9 @@ def literal_mapper(value): if raw_value == value: return type_arg + options = ", ".join(f"'{t}'" for t in type_args) raise ValueError( - f"Invalid choice: '{value}' (choose from {', '.join(str(t) for t in type_args)})" + f"Invalid choice: '{value}' (choose from literal values {options})" ) return literal_mapper @@ -142,20 +143,27 @@ def parse_union(*type_args: type) -> typing.Callable[[typing.Any], typing.Any]: def type_priority_key(type_) -> int: return type_priority.get(type_, 1) - mappers: list[typing.Callable] = [ - parse_value(t) for t in sorted(type_args, key=type_priority_key) + mappers: list[tuple[type, typing.Callable]] = [ + (t, parse_value(t)) for t in sorted(type_args, key=type_priority_key) ] def union_mapper(value): - for mapper in mappers: + exceptions = [] + for type_arg, mapper in mappers: try: return mapper(value) - except (ValueError, TypeError): - pass - - raise ValueError( - f"Could not parse '{value}' given options: {', '.join(backend_type(t) for t in type_args)}" - ) + except (ValueError, TypeError) as e: + if mapper is parse_none: + err = " - " + else: + err = f" - {repr_type(type_arg)}: {e}" + exceptions.append(err) + + # Perhaps we should be showing all failed mappings at some point. As-is, + # the preferred interpretation will be determined by order in the event + # of all mappings failing + reasons = "\n".join(exceptions) + raise ValueError(f"Possible variants\n{reasons}") return union_mapper @@ -172,16 +180,12 @@ def optional_mapper(value) -> T | None: return optional_mapper -def parse_none(): +def parse_none(value): """Create a value parser for None.""" + if value is None: + return - def map_none(value): - if value is None: - return - - raise ValueError(value) - - return map_none + raise ValueError(value) def parse_file_io( @@ -226,3 +230,10 @@ def detect_choices(origin: type, type_args: tuple[type, ...]) -> list[str] | Non def is_sequence_type(typ): return is_subclass(get_origin(typ) or typ, (typing.List, typing.Tuple, typing.Set)) + + +def repr_type(t): + if isinstance(t, type) and not typing.get_origin(t): + return str(t.__name__) + + return str(t).replace("typing.", "") diff --git a/src/cappa/command.py b/src/cappa/command.py index 9cb7094..959fbb2 100644 --- a/src/cappa/command.py +++ b/src/cappa/command.py @@ -227,7 +227,7 @@ def map_result(self, command: Command[T], prog: str, parsed_args) -> T: except Exception as e: exception_reason = str(e) raise Exit( - f"Invalid value for '{arg.names_str()}' with value '{value}': {exception_reason}", + f"Invalid value for '{arg.names_str()}': {exception_reason}", code=2, prog=prog, ) diff --git a/tests/arg/test_file_io.py b/tests/arg/test_file_io.py index 596f21f..29fbbd8 100644 --- a/tests/arg/test_file_io.py +++ b/tests/arg/test_file_io.py @@ -173,10 +173,8 @@ class Foo: with pytest.raises(cappa.Exit) as e: parse(Foo, "foo.py", backend=backend) - assert ( - e.value.message - == "Invalid value for 'bar' with value 'foo.py': can't have text and binary mode at once" - ) + assert str(e.value.message).startswith("Invalid value for 'bar'") + assert str(e.value.message).endswith("can't have text and binary mode at once") @backends diff --git a/tests/arg/test_literal.py b/tests/arg/test_literal.py index 6828568..5a09e97 100644 --- a/tests/arg/test_literal.py +++ b/tests/arg/test_literal.py @@ -1,10 +1,12 @@ from __future__ import annotations +import textwrap from dataclasses import dataclass from typing import Literal, Union import cappa import pytest +from typing_extensions import Annotated from tests.utils import backends, parse @@ -35,3 +37,23 @@ def test_invalid(backend): assert ( "invalid choice: 'thename' (choose from 'one', 'two', 'three', '4')" in message ) + + +@backends +def test_invalid_collection_of_literals(backend): + @dataclass + class Args: + foo: Annotated[set[Literal["one", "two"]] | None, cappa.Arg(short=True)] = None + + with pytest.raises(cappa.Exit) as e: + parse(Args, "-f", "three", backend=backend) + + assert e.value.code == 2 + + err = textwrap.dedent( + """\ + Invalid value for '-f': Possible variants + - set[Literal['one', 'two']]: Invalid choice: 'three' (choose from literal values 'one', 'two') + - """ + ) + assert str(e.value.message).lower() == err.lower() diff --git a/tests/arg/test_literal_intermixed_union.py b/tests/arg/test_literal_intermixed_union.py index 25d4086..ac57765 100644 --- a/tests/arg/test_literal_intermixed_union.py +++ b/tests/arg/test_literal_intermixed_union.py @@ -1,5 +1,6 @@ from __future__ import annotations +import textwrap from dataclasses import dataclass from typing import Literal, Union @@ -33,7 +34,10 @@ def test_invalid_string(backend): assert e.value.code == 2 - assert e.value.message == ( - "Invalid value for 'name' with value 'thename': " - "Could not parse 'thename' given options: , one" + err = textwrap.dedent( + """\ + Invalid value for 'name': Possible variants + - Literal['one']: Invalid choice: 'thename' (choose from literal values 'one') + - int: invalid literal for int() with base 10: 'thename'""" ) + assert err in str(e.value.message) diff --git a/tests/arg/test_mapping_failure.py b/tests/arg/test_mapping_failure.py index 03e3694..4ccbf7b 100644 --- a/tests/arg/test_mapping_failure.py +++ b/tests/arg/test_mapping_failure.py @@ -23,7 +23,7 @@ class ArgTest: assert e.value.code == 2 assert ( e.value.message - == "Invalid value for 'default' with value 'foo': invalid literal for int() with base 10: 'foo'" + == "Invalid value for 'default': invalid literal for int() with base 10: 'foo'" ) diff --git a/tests/arg/test_parse.py b/tests/arg/test_parse.py index c16155f..f2ce226 100644 --- a/tests/arg/test_parse.py +++ b/tests/arg/test_parse.py @@ -74,5 +74,5 @@ class ArgTest: assert e.value.code == 2 assert ( e.value.message - == "Invalid value for 'numbers' with value 'one': could not convert string to float: 'one'" + == "Invalid value for 'numbers': could not convert string to float: 'one'" )