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

gh-110525: Delete test_c_api method from set object #110688

Merged
merged 2 commits into from Oct 13, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 11, 2023

Now we have proper tests and this method can be deleted.

@sobolevn
Copy link
Member Author

I think that NEWS won't hurt in this case.

@rhettinger rhettinger removed their request for review October 13, 2023 00:32
@rhettinger
Copy link
Contributor

rhettinger commented Oct 13, 2023

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, git blame can no longer track these lines to their original commits or identify the person who made them.

@FFY00
Copy link
Member

FFY00 commented Oct 13, 2023

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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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.

Copy link
Member

@vstinner vstinner left a 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 vstinner merged commit 989a253 into python:main Oct 13, 2023
29 checks passed
@vstinner
Copy link
Member

@sobolevn: I hesitate to backport this one. Maybe keeping these tests in 3.12 is safer. What do you think?

@serhiy-storchaka
Copy link
Member

I would not backport it.

@vstinner
Copy link
Member

Thanks @sobolevn for your nice work!

@rhettinger:

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.

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
Copy link
Member

@rhettinger:

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.

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.

@rhettinger:

Also, git blame can no longer track these lines to their original commits or identify the person who made them.

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
Copy link
Member

Most C API tests are now written in Python, using thin wrappers from _testcapi. This makes it easier to extend tests and allows you to test more corner cases. Also you get better error reports.

Perhaps in future many wrappers could be generated automatically, that would reduce human factor and make writing new tests even easier.

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.

None yet

5 participants