gh-144087: Add support for unicode MINUS SIGN in int, float and complex#144095
gh-144087: Add support for unicode MINUS SIGN in int, float and complex#144095johnslavik wants to merge 31 commits intopython:mainfrom
int, float and complex#144095Conversation
|
LGTM. Needs tests and blurb. Maybe consider making the assignment a constant instead of a comment, though I don't feel strongly about it. |
Yep! Work in progress. Thanks for chiming in :) |
|
This also needs documentation. I would guess, it will take major part of the PR. |
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? |
Hardly this optimizes something.
Yes. _PyUnicode_TransformDecimalAndSpaceToASCII() used for floats and complexes (which gives another case of "sign in the middle") too. |
Some tests are failing!
- `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. |
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
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>
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
I think you can add here also tests for parsing floats in scientific notation, with exponent.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
6 participants
I don't see a different approach to solving this that is equally or more optimal in terms of trade-offs. If there's any alternative approach that I've missed, please let me know.