Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
vstinner merged 2 commits intopython:mainfromsobolevn:issue-110525-test_c_api
Oct 13, 2023

Conversation

@sobolevn
Copy link
Member

@sobolevnsobolevn commentedOct 11, 2023
edited by bedevere-appbot
Loading

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

@sobolevn
Copy link
MemberAuthor

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

@rhettingerrhettinger removed their request for reviewOctober 13, 2023 00:32
@rhettinger
Copy link
Contributor

rhettinger commentedOct 13, 2023
edited
Loading

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 commentedOct 13, 2023
edited
Loading

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-storchakaserhiy-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

@vstinnervstinner 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.

@vstinnervstinner merged commit989a253 intopython:mainOct 13, 2023
@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 reacted with thumbs up emoji

@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 asdismissive 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.

vstinner reacted with thumbs up emoji

Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@FFY00FFY00FFY00 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sobolevn@rhettinger@FFY00@vstinner@serhiy-storchaka

[8]ページ先頭

©2009-2025 Movatter.jp