-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-110525: Delete test_c_api
method from set
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I think that NEWS won't hurt in this case. |
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, |
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. |
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.
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.
@sobolevn: I hesitate to backport this one. Maybe keeping these tests in 3.12 is safer. What do you think? |
I would not backport it. |
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. |
Raymond, such comment can be seen as dismissive 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. |
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. |
Now we have proper tests and this method can be deleted.
set
andfrozenset
#110525