Skip to content

gh-111673: Refactor test_float/complex.py (split out support classes) #110956

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

Closed
wants to merge 20 commits into from

Conversation

skirpichev
Copy link
Member

@skirpichev skirpichev commented Oct 17, 2023

First commit mostly taken from #26827, second - some simple extension of this. @serhiy-storchaka

@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Oct 17, 2023
@skirpichev skirpichev changed the title Refactor test_float/complex.py (split out support classes) gh-111673: Refactor test_float/complex.py (split out support classes) Nov 3, 2023
@skirpichev skirpichev marked this pull request as draft November 3, 2023 10:02
@skirpichev skirpichev marked this pull request as ready for review November 4, 2023 03:47
@skirpichev

This comment was marked as resolved.

@AlexWaygood
Copy link
Member

@AlexWaygood, does this require news? I think - not.

Yeah, we can skip it for this one, good call

@skirpichev skirpichev marked this pull request as draft December 3, 2023 02:51
@skirpichev

This comment was marked as outdated.

@skirpichev skirpichev marked this pull request as ready for review June 1, 2024 02:12
@skirpichev
Copy link
Member Author

@sobolevn, maybe you can review this as an issue author? Let me know if it's better to split this to several pull requests.

@skirpichev skirpichev added the needs backport to 3.13 bugs and security fixes label Apr 17, 2025
@skirpichev
Copy link
Member Author

CC @vstinner

@skirpichev skirpichev requested a review from vstinner April 17, 2025 10:45
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.

I'm not sure that I like this change. It changes a lot of tests for little value (it's just refactoring). But I'm not against this change.

@serhiy-storchaka: Do you have an opinion on this change?

class FloatSubclass(float):
pass

class OtherFloatSubclass(float):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's worth it to have OtherFloatSubclass and OtherComplexSubclass in this number_helper module.

@serhiy-storchaka
Copy link
Member

I already proposed something similar in #84310. But it only defined classes with a single special method (__index__, __float__), which could be used in a number of tests. I am not sure that it is worth to share simple or too specialized classes like IntSubclass or WithIntAndIndex.

@skirpichev
Copy link
Member Author

Ok, thanks.

I think, I haven't enough motivation to push it further.

@skirpichev skirpichev closed this Apr 17, 2025
@skirpichev skirpichev deleted the refactor-fc-tests branch April 17, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review needs backport to 3.13 bugs and security fixes skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants