FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-144087: Add support for unicode MINUS SIGN in `int`, `float` and `complex` by johnslavik · Pull Request #144095 · python/cpython · GitHub

/ cpython Public

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
ConversationCommits (31)ChecksFiles changed
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 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 Bot mentioned this pull request Jan 21, 2026
Open

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

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

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 requested a review from encukou February 25, 2026 11:17
johnslavik requested review from picnixz and skirpichev March 2, 2026 00:01

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.

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread Doc/library/functions.rst Outdated
Comment thread Doc/library/functions.rst Outdated
encukou reviewed Mar 2, 2026
View reviewed changes
Comment thread Doc/library/functions.rst Outdated
Correct the grammar reference
f3dccd0 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
skirpichev reviewed Mar 2, 2026
View reviewed changes
Comment thread Doc/library/functions.rst Outdated
Keep character in grammar reference
9353f09 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
johnslavik requested a review from rhettinger March 12, 2026 14:20
Adjust docs to not say "ASCII"
3c2e361 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
skirpichev reviewed Mar 12, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 thread Doc/library/functions.rst Outdated
Comment thread Doc/library/functions.rst
Comment thread Doc/library/functions.rst Outdated
Comment thread Doc/library/stdtypes.rst Outdated
Comment thread Doc/library/functions.rst Outdated
Comment thread Lib/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)

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think you can add here also tests for parsing floats in scientific notation, with exponent.

johnslavik Mar 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, I'll address this on the weekend.

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

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 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 Bot added the stale Stale PR or inactive for long period of time. label May 5, 2026

johnslavik commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

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

johnslavik closed this Jul 20, 2026
johnslavik deleted the feat-handle-minus-sign branch July 21, 2026 01:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

encukou encukou left review comments
skirpichev skirpichev approved these changes
AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
picnixz Awaiting requested review from picnixz
serhiy-storchaka Awaiting requested review from serhiy-storchaka
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

Back | FazBrowse Home | New Git URL