GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44258: support PEP 515 for Fraction's initialization from string by skirpichev · Pull Request #26422 · python/cpython

Conversation

@skirpichev

@skirpichev skirpichev commented May 28, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue44258

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@skirpichev
bpo-44258: support PEP 515 for Fraction's initialization from string
aa8249a
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 28, 2021
@bedevere-bot bedevere-bot added the awaiting review label May 28, 2021
@skirpichev
regexps's version
8c96355
@mdickinson
mdickinson self-requested a review May 28, 2021 14:56
mdickinson
mdickinson reviewed May 28, 2021
View reviewed changes
Comment thread Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits May 29, 2021 04:10
@skirpichev
Merge branch 'main' into fix-44258
8c62c64
@skirpichev
A different regexps version, which doesn't suffer from catastrophic b…
38b16a9
…acktracking
mdickinson
mdickinson reviewed May 29, 2021
View reviewed changes
Comment thread Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed May 29, 2021
View reviewed changes
Comment thread Lib/fractions.py Outdated
denom = m.group('denom')
if denom:
denominator = int(denom)
denominator = m.group('den')

@mdickinson mdickinson May 29, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd suggest reverting this set of changes; IIUC they're not needed for this PR, and I don't see any particular advantage to the rename. (In fact, I prefer the version where we don't have the same local name referring to both a string and an int.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@skirpichev skirpichev May 30, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't see any particular advantage to the rename.

Naming of patterns seems to be more consistent in this case: num/den vs num/denom.

But you are right, this is not related to the PR. I'll revert.

I prefer the version where we don't have the same local name referring to both a string and an int

I don't think there are. We have den/denom for strings and denominator - which is an integer.

This patch may be better:

diff --git a/Lib/fractions.py b/Lib/fractions.py
index 180cd94c28..1268b6bd27 100644
--- a/Lib/fractions.py
+++ b/Lib/fractions.py
@@ -26,7 +26,7 @@
     (?=\d|\.\d)                           # lookahead for digit or .digit
     (?P<num>\d*|\d+(_\d+)*)               # numerator (possibly empty)
     (?:                                   # followed by
-       (?:/(?P<denom>\d+(_\d+)*))?        # an optional denominator
+       (?:/(?P<den>\d+(_\d+)*))?          # an optional denominator
     |                                     # or
        (?:\.(?P<decimal>d*|\d+(_\d+)*))?  # an optional fractional part
        (?:E(?P<exp>[-+]?\d+(_\d+)*))?     # and optional exponent
@@ -115,9 +115,9 @@ def __new__(cls, numerator=0, denominator=None, *, _normalize=True):
                     raise ValueError('Invalid literal for Fraction: %r' %
                                      numerator)
                 numerator = int(m.group('num') or '0')
-                denom = m.group('denom')
-                if denom:
-                    denominator = int(denom)
+                den = m.group('den')
+                if den:
+                    denominator = int(den)
                 else:
                     denominator = 1
                     decimal = m.group('decimal')

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
skirpichev added 4 commits May 30, 2021 06:05
@skirpichev
Merge branch 'main' into fix-44258
b131e51
@skirpichev
revert denom -> den
db9652e
@skirpichev
strip "_" from the decimal str, add few tests
6336aef
@skirpichev
drop redundant tests
b11ef1e
@skirpichev

skirpichev commented May 30, 2021

Copy link
Copy Markdown
Member Author

Artifact uploading failure seems to be unrelated.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson reviewed May 30, 2021
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks for the updates. LGTM! Please could you add a doc update? I think we need a .. versionchanged entry in the main docs, and a what's new entry.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
skirpichev added 2 commits May 30, 2021 12:17
@skirpichev
Add versionchanged & whatsnew entry
5f7e32a
@skirpichev
Amend Fraction constructor docs
983b67d
@skirpichev

skirpichev commented May 30, 2021

Copy link
Copy Markdown
Member Author

Please could you add a doc update? I think we need a .. versionchanged entry in the main docs, and a what's new entry.

I did. Lets see how it's...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mdickinson
mdickinson approved these changes May 31, 2021
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. One suggestion for rewording the .. versionchanged note.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/fractions.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 31, 2021
@skirpichev
Change .. versionchanged:...
076b396
@skirpichev

skirpichev commented May 31, 2021 via email

Copy link
Copy Markdown
Member Author
On Mon, May 31, 2021 at 04:38:28AM -0700, Mark Dickinson wrote: Could we reword or expand this to explicitly mention underscores? (Else someone reading the docs needs to either know what PEP 515 is, or follow the link to find out.) Maybe something along the lines of "Underscores are now permitted when creating a :class:Fraction instance from a string, following :PEP:515 rules."
Sure. Copied:) Sorry for not being too creative - your version seems fine for me.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson
mdickinson merged commit 89e50ab into python:main Jun 7, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 7, 2021
@skirpichev
skirpichev deleted the fix-44258 branch June 7, 2021 07:19
@skirpichev

skirpichev commented Jun 7, 2021

Copy link
Copy Markdown
Member Author

Oh, next time warn me about merging: commit message was a bit messy. If I did expect that - I would did rebase after every commit in the pr histoiry.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson mdickinson mentioned this pull request Sep 4, 2022
Fraction literals should support underscores in numbers and whitespace around numbers #96432
Closed
@mdickinson mdickinson mentioned this pull request Jan 13, 2024
Inconsistency in fractions.Fraction() initialization error with invalid string #114014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@mdickinson mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@skirpichev @mdickinson @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.