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

added isinstance and issubclass (tuple to type union) support #736

Closed
wants to merge 8 commits into from

Conversation

leshark
Copy link

@leshark leshark commented Oct 13, 2022

Small plugin for changing

isinstance(value, (type1, type2, type3, ...))
issubclass(value, (type1, type2, type3, ...))

to

isinstance(value, type1 | type2 | type3 | ...)
issubclass(value, type1 | type2 | type3 | ...)

as was enabled in pep-0604

Additionally added:

  • some stuff in .gitignore
  • tokenize-rt library to requirements-dev.txt (for some reason it was not present there. Maybe we should pin version?)

P.S: isinstance and issubclass have a weird signature:

__obj: object, __class_or_tuple: type | types.UnionType | tuple[type | types.UnionType | tuple[Any, ...], ...]

which means it is possible to pass a tuple of types inside tuple of types and it will work. I have never seen anyone using this, so I leave it as it is.

Copy link
Owner

@asottile asottile left a comment

Choose a reason for hiding this comment

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

idk that this is really an improvement -- I expect it to be slower too

.gitignore Outdated
@@ -2,3 +2,5 @@
*.pyc
/.coverage
/.tox
/venv
.python-version
Copy link
Owner

Choose a reason for hiding this comment

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

don't touch gitignore in projects you don't own

Copy link
Author

Choose a reason for hiding this comment

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

do you propose storing venv not in the project folder then?

Copy link
Owner

Choose a reason for hiding this comment

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

if you use virtualenv it is automatically gitignored

Copy link
Owner

Choose a reason for hiding this comment

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

or you can set a global ignore if you insist on using venv

personally I use tox --devenv venv

Copy link
Author

Choose a reason for hiding this comment

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

I will return it back, but can you explain why are you against of adding /venv to .gitignore?

Copy link
Owner

Choose a reason for hiding this comment

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

read the history

@@ -1,3 +1,4 @@
covdefaults>=2.1.0
coverage
pytest
tokenize-rt
Copy link
Owner

Choose a reason for hiding this comment

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

this indicates you're not running the project correctly. the dependencies for runtime are installed through setup.cfg, perhaps you're on a 5 year old version of tox or virtualenv?

Copy link
Author

Choose a reason for hiding this comment

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

removed

@leshark
Copy link
Author

leshark commented Oct 13, 2022

idk that this is really an improvement -- I expect it to be slower too

Do you want a benchmark? I doubt there is a big difference. Looks nicer though

@leshark leshark marked this pull request as ready for review October 13, 2022 16:44
@AlexWaygood
Copy link
Contributor

AlexWaygood commented Nov 17, 2022

idk that this is really an improvement -- I expect it to be slower too

isinstance() checks on Union types have been sped up significantly in 3.11, but they are indeed quite a bit slower than isinstance() checks on tuples on 3.10 (~4x slower). See python/cpython#91603 for more details.

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.

3 participants