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 shihai1991 requested review from markshannon and python/windows-team as code owners Aug 2, 2021
@shihai1991 shihai1991 requested a review from encukou Aug 2, 2021
@shihai1991
Copy link
Member Author

@shihai1991 shihai1991 commented Aug 2, 2021

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

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

This comment has been minimized.

@encukou

encukou Aug 3, 2021
Member

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?

This comment has been minimized.

@shihai1991

shihai1991 Aug 3, 2021
Author Member

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~

This comment has been minimized.

@encukou

encukou Aug 5, 2021
Member

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.

This comment has been minimized.

@shihai1991

shihai1991 Aug 5, 2021
Author Member

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 Aug 4, 2021
@shihai1991 shihai1991 force-pushed the shihai1991:bpo_42035_2 branch from 1b0e57c to 4cfebb2 Aug 7, 2021
@encukou
Copy link
Member

@encukou encukou commented Aug 17, 2021

Looks good, thanks!

@encukou encukou merged commit 3e2c643 into python:main Aug 17, 2021
13 checks passed
13 checks passed
@github-actions
Docs
Details
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
@github-actions
Address sanitizer Address sanitizer
Details
Azure Pipelines PR #20210807.8 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 42035 found
Details
@bedevere-bot
bedevere/news News entry found in Misc/NEWS.d
@shihai1991
Copy link
Member Author

@shihai1991 shihai1991 commented Aug 17, 2021

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
Projects
None yet
4 participants