Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-110525: Deletetest_c_api method fromset object#110688
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sobolevn commentedOct 11, 2023
I think that NEWS won't hurt in this case. |
rhettinger commentedOct 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I won't block any of this but view it all as unnecessary code churn (no user's life is now better). Also, when I was actively developing the C API for sets, it was convenient to have the caller and callee in the same file. Also, |
FFY00 commentedOct 13, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Even though its value is subjective, this work does make the code nicer, and I think it's probably more beneficial to have a contributor do this kind of work, gaining experience in the codebase, than any of the minor drawbacks you mention. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If Raymond have no objections, then LGTM.
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. I always found that it was a surprising place for tests, but I didn't touch it since it didn't bother me enough. The new C API tests are way more complete, cover more cases. These old tests are now redundant.
vstinner commentedOct 13, 2023
@sobolevn: I hesitate to backport this one. Maybe keeping these tests in 3.12 is safer. What do you think? |
serhiy-storchaka commentedOct 13, 2023
I would not backport it. |
vstinner commentedOct 13, 2023
Thanks@sobolevn for your nice work!
Right, in the past, we wrote C API tests differently. But for 1 or 2 years, all C API tests are being moved to test_capi. I don't fully agree, but I'm fine with this. I'm now trying to help to make tests consistent. Adding more C API tests is always a great thing. Nikita spent significant time to enhance C API tests for the set and frozenset tests. He worked hard for that. His work was reviewed by multiple core devs and added recently to test_capi. It increases the code coverage and test coverage, it's a nice enhancement. |
vstinner commentedOct 13, 2023
Raymond, such comment can be seen asdismissive and non supportive. While you can see it as unnecessary, other core devs who reviewed and merged these C API test changes.
It's just a normal fact of contributing to open source project, and IMO it's a great thing that code get updated by other people to make it even better. Maybe you can use your Python expertise to help contributors to get their work merged. |
serhiy-storchaka commentedOct 13, 2023
Most C API tests are now written in Python, using thin wrappers from Perhaps in future many wrappers could be generated automatically, that would reduce human factor and make writing new tests even easier. |
Uh oh!
There was an error while loading.Please reload this page.
Now we have proper tests and this method can be deleted.
setandfrozenset#110525