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

Disallow TypeAlias type #3201

Merged
merged 12 commits into from Dec 30, 2021
Merged

Disallow TypeAlias type #3201

merged 12 commits into from Dec 30, 2021

Conversation

sobolevn
Copy link
Member

I've created two functions is_forbidden_to_register and is_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

@sobolevn
Copy link
Member Author

Btw, I am pretty sure that Catholic Christmas is celebrated in Australia, so merry cristmas to @Zac-HD 🌲

@sobolevn sobolevn requested a review from Zac-HD December 26, 2021 12:03
@Zac-HD
Copy link
Member

Zac-HD commented Dec 27, 2021

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...

Copy link
Member

@Zac-HD Zac-HD left a 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 😅

hypothesis-python/RELEASE.rst Show resolved Hide resolved
@sobolevn
Copy link
Member Author

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

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! 👍

@sobolevn
Copy link
Member Author

@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 hypothesis.extra.redis.RedisExampleDatabase seems to still be there.

Copy link
Member

@Zac-HD Zac-HD left a 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).

hypothesis-python/docs/changes.rst Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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 😁

Copy link
Member Author

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:
Снимок экрана 2021-12-29 в 15 52 30

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?

Copy link
Member Author

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.

@@ -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'
Copy link
Member Author

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?

@sobolevn
Copy link
Member Author

sobolevn commented Dec 29, 2021

@Zac-HD done! Only one unrelated doc failure is left.

Moved in upstream docs reorg; we'll change back if this is reverted.
Copy link
Member

@Zac-HD Zac-HD left a 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!

@Zac-HD Zac-HD merged commit 76847e3 into master Dec 30, 2021
@Zac-HD Zac-HD deleted the disallow-type-alias branch December 30, 2021 03:27
@sobolevn
Copy link
Member Author

Thank you for the review! I still have several extra types to forbid, I will send new PRs soon 🙂

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

2 participants