gh-144087: Add support for unicode MINUS SIGN in `int`, `float` and `complex` by johnslavik · Pull Request #144095 · python/cpython
LGTM. Needs tests and blurb. Maybe consider making the assignment a constant instead of a comment, though I don't feel strongly about it.
LGTM. Needs tests and blurb.
Yep! Work in progress.
Thanks for chiming in :)
This also needs documentation. I would guess, it will take major part of the PR.
I don't see a different approach to solving this that is equally or more optimal in terms of trade-offs
How about only checking for the first non whitespace character? signs should not appear in the middle of a number unless they are in the e/E/p suffix I guess?
How about only checking for the first non whitespace character?
Hardly this optimizes something.
unless they are in the e/E/p suffix I guess?
Yes. _PyUnicode_TransformDecimalAndSpaceToASCII() used for floats and complexes (which gives another case of "sign in the middle") too.
- `CAPIFloatTest.test_fromstring` doesn't test for minus signs - I can't find `complex` C API tests
I think that reads better
This seems like something we could add to the "What's New" log. I'll do that.
@picnixz, @skirpichev, I tagged you for reviews because I thought you may be interested now that it is ready :-)
I'm particularly unsure about whether I missed or added unnecessary tests, and whether the documentation changes are good.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with minor nitpicks.
Maybe you can do docs more compact. See suggestions as an example.
Alternatively, omit just "(ASCII plus sign)" clarifications. There shouldn't be much confusion. Visual difference for '-' and '−' is less apparent.
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also, that we have a separate parsing from string code in the fromhex() class method.
I don't see good reasons to reject new sign character here, if we are going to support in the float constructor.
| def test_float(self): | ||
| self.assertEqual(float(3.14), 3.14) | ||
| self.assertEqual(float(314), 314.0) | ||
| self.assertEqual(float(" 3.14 "), 3.14) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can add here also tests for parsing floats in scientific notation, with exponent.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll address this on the weekend.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Please do not merge this. I need to collect more feedback on the idea.
This PR is stale because it has been open for 30 days with no activity.