Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-108303: Move all math files to Lib/test/mathdata/#109512
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sobolevn commented Sep 17, 2023 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
I dislike "data" directories.
It seems like test_math uses 3 data files which are only used by test_math.
test_float uses 2 data files which are only used by test_float.
For me, "data" directories are nice when data files are shared by different tests. It doesn't seem to be the case here.
What do you think of creating test_float/ and test_math/ directories instead?
sobolevn commented Sep 18, 2023
I don't have any strong opinions here, for me Why do I prefer the first option? Because later some other file can be added there: possibly from |
serhiy-storchaka commented Sep 18, 2023
"cmath_testcases.txt" is shared with |
vstinner commented Sep 18, 2023
test_cmath uses dark black magic, it uses class inheritance: So it doesn't directly reuses "cmath_testcases.txt", but uses it "via" test_math. Oh, that's "surprising". |
vstinner commented Sep 18, 2023
@serhiy-storchaka approved the change, I'm not strongly against I'm just surprised that 2 test files are added there whereas they are only used by test_float. I don't see the point of "sharing" these two files. Maybe make a test_float/ directory for them? @serhiy-storchaka: If you're fine with mathdata/ approach,go ahead and merge the PR :-)
That sounds to be be an hypothetical use case. I would prefer to stick to current test data files usage to decide how to organize them. |
serhiy-storchaka commented Sep 18, 2023
It also imports the path to the test data file and the parsing function from |
vstinner commented Sep 18, 2023
I can see the point of having mathdata/ for test_math and test_cmath. What about test_float test files? |
serhiy-storchaka commented Sep 18, 2023
They could be used in tests for |
vstinner commented Sep 18, 2023
If tomorrow other tests start using test_float data files, we can re-consider the organization of these data files, no? Are you proposing to make this change right now?
You can easily test: remove data files, run tests, do they still pass? :-) The purpose of #108303 is to clarify how test data files are used ;-) |
rhettinger commented Sep 18, 2023
To me, these kind of edits (mostly rearrangements) have no value. It is mostly just churn. |
serhiy-storchaka commented Sep 18, 2023
|
vstinner commented Sep 19, 2023
Oh, @serhiy-storchaka made #109548 which makes mathdata/ directory more relevant :-) |
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.
miss-islington commented Sep 21, 2023
miss-islington commented Sep 21, 2023
miss-islington commented Sep 21, 2023
Sorry, @sobolevn and @vstinner, I could not cleanly backport this to |
miss-islington commented Sep 21, 2023
Sorry, @sobolevn and @vstinner, I could not cleanly backport this to |
This one is pretty straight-forward. All math data now lives in a specialized place.