-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add more internal type hints #4105
Conversation
d7139b4
to
0b47b9c
Compare
@cacheable | ||
@defines_strategy(force_reusable_values=True) | ||
def characters( | ||
*, | ||
codec: Optional[str] = None, | ||
min_codepoint: Optional[int] = None, | ||
max_codepoint: Optional[int] = None, | ||
categories: Optional[Collection[CategoryName]] = None, | ||
exclude_categories: Optional[Collection[CategoryName]] = None, | ||
categories: Optional[Categories] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notably this is a loosening of the type, from Collection[CategoryName]
to Iterable[CategoryName]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, not sure about this - it works, but non-collection iterables seem like a real footgun for our users!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something something hyrum's Law / may as well type what works? I think typing as Iterable
doesn't push the footgun up to the user, so to speak – they would have to be shooting themselves already. More like Collection
forces them to "fix" what in our determination is a "footgun" (but maybe they have a valid-but-cursed custom iterable type that doesn't implement e.g. __contains__
).
But I am very unexperienced in maintaining forward-facing types, so I'm happy to defer this decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stick with the status quo (Collection
) here, and get the rest in 🙂
hmm, I've been testing locally with |
d09a568
to
3ecc31b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--allow-redefinition
seems fine to me
@@ -206,14 +218,14 @@ def get_type_hints(thing): | |||
# We still use the same trick on Python 3, because Numpy values and other | |||
# custom __floor__ or __ceil__ methods may convert via floats. | |||
# See issue #1667, Numpy issue 9068. | |||
def floor(x): | |||
def floor(x: float) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's broader than this though; essentially typing.SupportsInt
(& Number
, except no intersection types)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we defer this until a usage site requires it? I don't want to write a type which is subtly an overapproximation, whereas I am confident that float is an underapproximation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do use this on Numpy scalars, so maybe we already need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a type-checked usage site. I did just look into typing this though and it's a rabbit hole of defining a custom protocol inheriting SupportsInt
and from _typeshed import SupportsAllComparisons
except then mypy errors with Non-overlapping equality check
and I gave up. Let's revert and come back another day?
# we can't use this at runtime until from_type supports | ||
# protocols -- breaks ghostwriter tests | ||
class RandomLike(Protocol): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me, later: maybe all this takes is to mark this protocol as runtime_checkable?
@cacheable | ||
@defines_strategy(force_reusable_values=True) | ||
def characters( | ||
*, | ||
codec: Optional[str] = None, | ||
min_codepoint: Optional[int] = None, | ||
max_codepoint: Optional[int] = None, | ||
categories: Optional[Collection[CategoryName]] = None, | ||
exclude_categories: Optional[Collection[CategoryName]] = None, | ||
categories: Optional[Categories] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, not sure about this - it works, but non-collection iterables seem like a real footgun for our users!
yeah, sorry, I haven't had time to finish this pr up. Happy to do whatever's most efficient here. |
oh, no worries at all about dropping stuff, just look at how old some of my PRs are getting! If working on Hypothesis would feel like an obligation instead of a fun hobby, that's a good time to go do something else instead 🙂 |
I can't even tell you how much I enjoy + wish I had time to work on hypothesis 😄 But, the paper takes priority for the moment. |
I've brought this branch up to date, and removed some overcomplicated typing in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Comments below, then merge at will.
(unfortunately I just watched that no-longer-xfailed mypy test flake, so maybe we need to revert 😭)
@@ -206,14 +218,14 @@ def get_type_hints(thing): | |||
# We still use the same trick on Python 3, because Numpy values and other | |||
# custom __floor__ or __ceil__ methods may convert via floats. | |||
# See issue #1667, Numpy issue 9068. | |||
def floor(x): | |||
def floor(x: float) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do use this on Numpy scalars, so maybe we already need it?
@cacheable | ||
@defines_strategy(force_reusable_values=True) | ||
def characters( | ||
*, | ||
codec: Optional[str] = None, | ||
min_codepoint: Optional[int] = None, | ||
max_codepoint: Optional[int] = None, | ||
categories: Optional[Collection[CategoryName]] = None, | ||
exclude_categories: Optional[Collection[CategoryName]] = None, | ||
categories: Optional[Categories] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's stick with the status quo (Collection
) here, and get the rest in 🙂
Saw the same mypy flake in #4163, so I've put back the xfail here |
26e0a0b
to
ba09d86
Compare
ba09d86
to
ad455e4
Compare
Largely focused on non-conjecture this time. There are some source changes here as I tightened things up while typing, like no longer passing length-one strings or
None
toas_general_categories
.