Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NonEmpty[str] type cannot be used as sorting key #207

Closed
flaeppe opened this issue Apr 27, 2022 · 4 comments
Closed

NonEmpty[str] type cannot be used as sorting key #207

flaeppe opened this issue Apr 27, 2022 · 4 comments

Comments

@flaeppe
Copy link
Contributor

flaeppe commented Apr 27, 2022

Not sure if I'm using NonEmpty incorrectly here with str. But the code below does not typecheck, and I think it should? (Since the underlying data type is a string)

from typing import NamedTuple
from phantom.sized import NonEmpty


class X(NamedTuple):
    value: NonEmpty[str]

sorted(
    [X(value=NonEmpty[str].parse("first")), X(value=NonEmpty[str].parse("second"))],
    key=lambda x: x.value
)

Results in the following error

10: error: Argument "key" to "sorted" has incompatible type "Callable[[X], NonEmpty[str]]"; expected "Callable[[X], Union[SupportsDunderLT, SupportsDunderGT]]"  [arg-type]

10: error: Incompatible return value type (got "NonEmpty[str]", expected "Union[SupportsDunderLT, SupportsDunderGT]")  [return-value]
@antonagestam
Copy link
Owner

antonagestam commented Apr 27, 2022

@flaeppe NonEmpty[str] is a subtype of Sized and Iterable[str], but not a subtype of str. If you want a NonEmpty-type that is also a subtype of str you'd need to implement that like this:

# This has the added benefit that you can provide minCharacters in the schema
# which is distinct to minItems in OpenAPI! (Learned this recently).
class NonEmptyStr(str, NonEmpty[str]):
    ...

PhantomSized is generic in T, which is passed to SizedIterable which is generic in another T that is passed to Iterable. So the type argument to NonEmpty dictates the type of the value you'd get from next(NonEmpty()), it doesn't alter the super-type of the subscripted type :)

@antonagestam
Copy link
Owner

I think you've made the same assumption as the OP of this issue: #160 (comment)

I guess this is a good indication documentation could be improved.

@flaeppe
Copy link
Contributor Author

flaeppe commented Apr 27, 2022

Thanks for the clarification! That makes sense.

@antonagestam
Copy link
Owner

@flaeppe I'll close this issue for now but feel free to suggest ways to improve this 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants