bpo-44258: support PEP 515 for Fraction's initialization from string

[skirpichev]

https://bugs.python.org/issue44258

[mdickinson]

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

[skirpichev]

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')

[skirpichev]

Artifact uploading failure seems to be unrelated.

[mdickinson]

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.

[skirpichev]

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

[mdickinson]

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

[skirpichev]

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.

[skirpichev]

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.