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

bpo-42035: Add a PyType_GetQualName() to get type's qualified name. #27551

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Aug 2, 2021

@shihai1991
Copy link
Member Author

@encukou Hi, Petr. Would you mind to take a look :)

Comment on lines +1178 to +1179
if (PyObject_SetAttrString(HeapTypeNameType,
"__qualname__", spec_name) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting test.
Would it make sense to test the original qualname first, then assign a different one and test that PyType_GetQualName returns the replacement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha. I try to get some qual names in _testcapimodule. But I can get the qual name with prefix.
The reason is res->ht_qualname = res->ht_name; in https://github.com/python/cpython/blob/main/Objects/typeobject.c#L3439.
I don't know the reason why the qual name need to reomve the prefix too.
So I set a qual name with prefix to test this C API, somthing like a mock behavior~

Copy link
Member

Choose a reason for hiding this comment

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

Right, C-API classes don't have distinct qualnames.
IMO, the best thing to do here would be:

  • Test the original qualname (same as the short name)
  • Change the qualname
  • Then test the new qualname.

Ideally, the same thing would be tested for PyType_GetName as well – it's not that different from this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, C-API classes don't have distinct qualnames.
IMO, the best thing to do here would be:

  • Test the original qualname (same as the short name)
  • Change the qualname
  • Then test the new qualname.

Make sense. How about this updated PR :)

Ideally, the same thing would be tested for PyType_GetName as well – it's not that different from this function.

Got it. I will update enhance this test case later.

@shihai1991 shihai1991 requested a review from encukou August 4, 2021 07:54
@encukou
Copy link
Member

encukou commented Aug 17, 2021

Looks good, thanks!

@encukou encukou merged commit 3e2c643 into python:main Aug 17, 2021
@shihai1991
Copy link
Member Author

Looks good, thanks!

Thanks a million, petr :)

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.

4 participants