◐ Shell
reader mode source ↗
Skip to content

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

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

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

@johnslavik johnslavik 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-app bedevere-app Bot 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
Member Author

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
9 hidden items Load more…
@johnslavik

johnslavik commented Feb 21, 2026

Copy link
Copy Markdown
Member Author

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

@johnslavik johnslavik requested a review from encukou February 25, 2026 11:17
@johnslavik johnslavik requested review from picnixz and skirpichev March 2, 2026 00:01
@johnslavik

johnslavik commented Mar 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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
skirpichev approved these changes Mar 2, 2026
View reviewed changes

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

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

@johnslavik @encukou
Correct the grammar reference …
f3dccd0 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@johnslavik @skirpichev
Keep character in grammar reference …
9353f09 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@johnslavik johnslavik requested a review from rhettinger March 12, 2026 14:20
@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

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

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

@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
Member Author

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

@github-actions

github-actions Bot commented May 5, 2026

Copy link
Copy Markdown

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

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments

@skirpichev skirpichev skirpichev approved these changes

@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

@picnixz picnixz Awaiting requested review from picnixz

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@rhettinger rhettinger Awaiting requested review from rhettinger

Assignees

No one assigned

Labels

awaiting core review DO-NOT-MERGE stale Stale 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