Skip to content

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