-
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
Disallow TypeAlias
type
#3201
Disallow TypeAlias
type
#3201
Conversation
Btw, I am pretty sure that Catholic Christmas is celebrated in Australia, so merry cristmas to @Zac-HD 🌲 |
Thanks 💖 Yeah, we celebrate on (or around 😅) Dec. 25th, so I've just had two huge lunches with extended family. The entire country basically shuts down for a holiday from ~20 Dec to ~5 Jan (or ~28 Jan, after Australia Day, for school holidays!) -- it's pretty similar to July in Europe or Canada. White Wine in the Sun captures the mood pretty well, especially since I'm moving in a few weeks... |
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.
Basically looks good to me 😁
...fiddly comments below though 😅
We are going to start celebrating New Year and then go for two full weeks of holidays, including Ortodox Cristmas on 7th of Jan, we stop thinking "it is still holidays" on "Old New Year" day. One of the most favourite times of the year! ❄️ I will fix all comments later today, thanks a lot for the review! 👍 |
@Zac-HD can you please take a look? https://github.com/HypothesisWorks/hypothesis/runs/4657300352?check_suite_focus=true This does not seem related. And |
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 looks like the redis
warning in our docs build is because they moved the docs upstream! See redis/redis-py#1839; if this doesn't get fixed upstream we'll need to add a .client
into the relevant docstring in hypothesis.extra.redis
(ends up here).
@@ -1023,6 +1023,9 @@ def as_strategy(strat_or_callable, thing, final=True): | |||
"strings." | |||
) | |||
raise InvalidArgument(f"thing={thing!r} must be a type") # pragma: no cover | |||
if thing in types.TypeAliasTypes: # pragma: no cover |
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 add the typing_extensions
tests to our coverage runs. It's a small perf hit, but I'll be happier when I can see that these branches are definitely covered 😁
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.
But, typing_extensions
is not available during coverage
. It is only used right now in test_backported_types
:
It is controlled by:
pip install 'typing_extensions>=4.0.0'
$PYTEST tests/typing_extensions/
if [ "$(python -c 'import sys; print(sys.version_info[:2] == (3, 7))')" = "False" ] ; then
# Required by importlib_metadata backport, which we don't want to break
pip uninstall -y typing_extensions
fi
Do you want me to change this?
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.
Another solution is to create python>=3.10 only file with typing.TypeAlias
test.
hypothesis-python/tests/typing_extensions/test_backported_types.py
Outdated
Show resolved
Hide resolved
@@ -31,7 +31,7 @@ pip install fakeredis | |||
$PYTEST tests/redis/ | |||
pip uninstall -y redis fakeredis | |||
|
|||
pip install 'typing_extensions!=3.10.0.1' | |||
pip install 'typing_extensions>=4.0.0' |
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.
Do we need this version info?
@Zac-HD done! Only one unrelated doc failure is left. |
Moved in upstream docs reorg; we'll change back if this is reverted.
23434d8
to
7472782
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.
I pushed tiny patches for coverage and for the docs error 👍 Let's ship it!
Thank you for the review! I still have several extra types to forbid, I will send new PRs soon 🙂 |
I've created two functions
is_forbidden_to_register
andis_forbidden_to_dispatch
.I am planning to refactor them to match our rules. For example, we can dispatch
Annotated
type, but it does not make sense with.register_type_strategy
.Refs #2978