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

pytypeinfo: rename is_instance to is_type_of #1278

Merged
merged 1 commit into from
Nov 15, 2020

Conversation

davidhewitt
Copy link
Member

While working on #1276 I realised that PyTypeInfo::is_instance is actually a bit misleading.

The default implementation of it uses PyObject_TypeCheck, which is similar to isinstance() but doesn't call the __isinstance__ hook.

So I think it's better to make this name clearer. I went for check and check_exact, inspired by PyObject_TypeCheck.

This is a little breaking, but I think in practice this should only affect people who are manually implementing PyTypeInfo (hopefully very few users).

@alex
Copy link
Contributor

alex commented Nov 15, 2020

What do you think about type_check and type_check_exact?

@davidhewitt
Copy link
Member Author

Yeah that's probably even clearer!

@kngwyu
Copy link
Member

kngwyu commented Nov 15, 2020

I understand the motivation, but I still prefer the is_ prefix.

@davidhewitt
Copy link
Member Author

I understand the motivation, but I still prefer the is_ prefix.

I do agree is_ is nicer. I'd really like to move away from is_instance now that I'm aware this is subtly different to the other is_instance functions. Otherwise I think we're asking for people to write bugs by accident!

How about T::is_type_of and T::is_exact_type_of ?

I think that reads quite naturally - e.g. PyList::is_type_of(list) compared to the original PyList::is_instance(list).

@davidhewitt
Copy link
Member Author

Changed to is_type_of and is_exact_type_of. @kngwyu would you be ok with these names?

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Looks nice 👍

@davidhewitt davidhewitt force-pushed the pytypeinfo-check branch 2 times, most recently from e6871a6 to cba652b Compare November 15, 2020 18:33
@davidhewitt davidhewitt changed the title pytypeinfo: rename is_instance to check pytypeinfo: rename is_instance to is_type_of Nov 15, 2020
@davidhewitt davidhewitt merged commit aa30163 into PyO3:master Nov 15, 2020
@davidhewitt davidhewitt deleted the pytypeinfo-check branch December 24, 2021 02:06
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