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

use type aliases for shorthand constraints #6

Merged
merged 2 commits into from May 18, 2022

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented May 5, 2022

#5 (comment)

I think these names make more sense when they're wrapping the type, but I'm happy to keep the old ones

@adriangb adriangb requested a review from samuelcolvin May 5, 2022 04:18
@adriangb adriangb changed the title use type aliases for shortcut constraints use type aliases for shorthand constraints May 5, 2022
@adriangb adriangb mentioned this pull request May 5, 2022
Copy link
Contributor

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pro this, but we should be explicit in docs and tests about whether these work with bytes and bytearray.

@adriangb
Copy link
Contributor Author

adriangb commented May 17, 2022

we should be explicit in docs and tests about whether these work with bytes and bytearray

I may be missing something but what is special about bytes/bytearray? Oh I see, I am explicitly referencing strings.

@adriangb
Copy link
Contributor Author

So firstly I think your comment generally applies regardless of using type aliases or not: str.islower as a predicate applied to bytes makes no sense. The only difference is that with this change that would be enforced by the type system (I'm giving the typevar a bound).

Here are a couple paths forward for this:

Only provide a predicate for str

All we have to do is explain in the docs that we only provide predicates for str.
I think this is the easiest option.
We can merge this and then rebase #7 or vice versa.

Multiple predicates

Have predicates for str.islower, bytes.islower and bytes.islower.

Differentiated by name

This is in line with what Pydantic does (constr, conbytes)

from annotated_types import LowerCaseStr, LowerCaseBytes

foo: LowerCaseStr[str]
bar: LowerCaseBytes[bytes]

Namespaces

Similar to the above, but instead of tacking on a suffix we namespace the variations.

from annotated_types.strings import LowerCase
from annotated_types.bytes import LowerCase

Introduce logic for handling non applicable predicates

That is, we would define our shorthand as:

T = TypeVar("T", bound=Union[str, bytes])
LowerCase = Annotated[T, Predicate(str.islower), Predicate(bytes.islower)]

foo: LowerCase[str]
bar: LowerCase[bytes]

And say something like "predicates that can't be applied to the type in question should be ignored".

I don't particularly like this, I think it will make things harder to reason about and parse.
Generally this should be expressed as:

lowercase_str_or_bytes: Union[Annotated[str, Predicate(str.islower)], Annotated[bytes, Predicate(bytes.islower)]

But that's super verbose and we can't provide a type alias for it.

@Zac-HD
Copy link
Member

Zac-HD commented May 18, 2022

Just some very quick thoughts because it's rather late here:

  • No opinion at this time on whether we should provide IsLowerBytes or whatever we'd call it.
  • If we do provide it, separate names is IMO the way to go; contra PEP-20 we don't actually need namespaces here 😂
  • I'm also perfectly happy only defining these short aliases for strings. Treating bytes as containing human-readable text is a bit of an antipattern in Python, or for that matter any system that cares about any natural language other than english.
  • For similar reasons, I'm just fine if it's super awkward to annotate APIs that freely mix text and bytestrings. (there are reasonable exceptions like "is a json document", but for that you just deserialize it, not check the serialized form)

And say something like "predicates that can't be applied to the type in question should be ignored".

This would need to be "we do not specify semantics for predicates which can't be applied to the type in question" - Hypothesis is just going to turn Annotated[X, Predicate(pred)] into from_type(X).filter(pred). In some cases it's rewritten to a more efficient form internally, but the standard behavior is to invoke the function and I really don't want to add special handling for errors in some predicates.

@samuelcolvin
Copy link
Contributor

Ok, then let's just leave it as is and as a note to the docs.

@adriangb
Copy link
Contributor Author

On board with only providing shorthands for str. @Zac-HD do you want me to merge this and update #7 or the other way around?

@Zac-HD Zac-HD merged commit 25c888a into annotated-types:main May 18, 2022
@Zac-HD
Copy link
Member

Zac-HD commented May 18, 2022

I'll get the docs... Hopefully tonight, and then we can merge!

@adriangb adriangb deleted the alias branch May 18, 2022 16:02
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

Successfully merging this pull request may close these issues.

None yet

3 participants