Skip to content

gh-144087: Add support for unicode MINUS SIGN in int, float and complex - #144095

Closed
johnslavik wants to merge 31 commits into
python:mainfrom
johnslavik:feat-handle-minus-sign
Closed

gh-144087: Add support for unicode MINUS SIGN in int, float and complex#144095
johnslavik wants to merge 31 commits into
python:mainfrom
johnslavik:feat-handle-minus-sign

Conversation

@johnslavik

@johnslavikjohnslavik commented Jan 21, 2026•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

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.

@bedevere-appbedevere-appBot mentioned this pull request Jan 21, 2026
Open
@jaraco

jaraco commented Jan 24, 2026

Copy link
Copy Markdown
Member

LGTM. Needs tests and blurb. Maybe consider making the assignment a constant instead of a comment, though I don't feel strongly about it.

@johnslavik

johnslavik commented Jan 24, 2026•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

LGTM. Needs tests and blurb.

Yep! Work in progress.

Thanks for chiming in :)

@skirpichev

skirpichev commented Jan 24, 2026

Copy link
Copy Markdown
Member

This also needs documentation. I would guess, it will take major part of the PR.

@picnixz

picnixz commented Jan 28, 2026

Copy link
Copy Markdown
Member

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?

@skirpichev

skirpichev commented Jan 28, 2026

Copy link
Copy Markdown
Member

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.

@johnslavik
Add some test coverage
d3f7c80
Some tests are failing!
@johnslavik
Fix test
3fe1b82
@johnslavik
Add news entry
973e191
@johnslavik
Add C API test coverage (only test_long_fromunicodeobject)
2695c06
- `CAPIFloatTest.test_fromstring` doesn't test for minus signs
- I can't find `complex` C API tests
@johnslavik
Use :func: in news entry for better display
cf0324a
@johnslavik
Update int docs
3855919
@johnslavik
Update float docs
ae6cd6c
@johnslavik
Update complex docs
4a78b3a
@johnslavik
Simplify complex complex docs
f233ebd
@johnslavik
Fix formatting
03d24b2
@johnslavik
Update float syntax snippet
5ce17c9
@johnslavik
Update stdtypes reference
b4b9574
@johnslavik
Clarify complex docs
ff013f5
@johnslavik
Slightly changed "syntax" used in float production list
ca6eca0
@johnslavik
Fix alternative used in complex docs, I think it was incorrect
26dadc5
@johnslavik
Parenthesize consistently
6391d27
I think that reads better
@johnslavik
Parenthesize consistently vol. 2
f7c3fa7
@johnslavik
Empty commit to try to skip incremental lint
0d519f4
@johnslavik
Test NaNs properly
6c5994a
@johnslavik
Fix trailing whitespace (was confused)
0e7110c
@johnslavik
Parenthesize in news entry too
199a879
@johnslavik
Use .. versionchanged:: next
a9874e7
@johnslavik
johnslavik marked this pull request as ready for review February 21, 2026 10:46
@johnslavik

johnslavik commented Feb 21, 2026

Copy link
Copy Markdown
MemberAuthor

This seems like something we could add to the "What's New" log. I'll do that.

@johnslavik

johnslavik commented Mar 2, 2026•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

johnslavik
johnslavik commented Mar 2, 2026
View reviewed changes
Comment threadLib/test/test_int.py
skirpichev
skirpichev approved these changes Mar 2, 2026
View reviewed changes

@skirpichevskirpichev left a comment

Copy link
Copy Markdown
Member

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.

Comment threadDoc/library/functions.rst Outdated
Comment threadDoc/library/functions.rst Outdated
encukou
encukou reviewed Mar 2, 2026
View reviewed changes
Comment threadDoc/library/functions.rst Outdated
@johnslavik@encukou
Correct the grammar reference
f3dccd0
Co-authored-by: Petr Viktorin <encukou@gmail.com>
skirpichev
skirpichev reviewed Mar 2, 2026
View reviewed changes
Comment threadDoc/library/functions.rst Outdated
@johnslavik@skirpichev
Keep character in grammar reference
9353f09
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@ambv@skirpichev
Adjust docs to not say "ASCII"
3c2e361
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
skirpichev
skirpichev reviewed Mar 12, 2026
View reviewed changes

@skirpichevskirpichev left a comment

Copy link
Copy Markdown
Member

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.

Comment threadDoc/library/functions.rst Outdated
Comment threadDoc/library/functions.rst
Comment threadDoc/library/functions.rst Outdated
Comment threadDoc/library/stdtypes.rst Outdated
Comment threadDoc/library/functions.rst Outdated
Comment threadLib/test/test_float.py
def test_float(self):
self.assertEqual(float(3.14), 3.14)
self.assertEqual(float(314), 314.0)
self.assertEqual(float(" 3.14 "), 3.14)

@skirpichevskirpichevMar 12, 2026

Copy link
Copy Markdown
Member

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.

@johnslavikjohnslavikMar 13, 2026

Copy link
Copy Markdown
MemberAuthor

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.

@johnslavik@skirpichev
Remove more mentions of other signs being ASCII from docs
301eb4b
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@johnslavik

johnslavik commented Mar 24, 2026

Copy link
Copy Markdown
MemberAuthor

Please do not merge this. I need to collect more feedback on the idea.

@github-actions

github-actionsBot commented May 5, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label May 5, 2026
@johnslavik

johnslavik commented Jul 20, 2026

Copy link
Copy Markdown
MemberAuthor

I'm not sure it is worth it. Feel free to pick this up if you want!

@johnslavik
johnslavik deleted the feat-handle-minus-sign branch July 21, 2026 01:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukouencukouencukou left review comments
@skirpichevskirpichevskirpichev approved these changes
@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner
@picnixzpicnixzAwaiting requested review from picnixz
@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka
@rhettingerrhettingerAwaiting requested review from rhettinger

Assignees

No one assigned

Labels

awaiting core reviewDO-NOT-MERGEstaleStale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@johnslavik@jaraco@skirpichev@picnixz@encukou@ambv