bpo-44258: support PEP 515 for Fraction's initialization from string - #26422
bpo-44258: support PEP 515 for Fraction's initialization from string#26422mdickinson merged 11 commits into
Conversation
Sorry, something went wrong.
| denom = m.group('denom') | ||
| if denom: | ||
| denominator = int(denom) | ||
| denominator = m.group('den') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Artifact uploading failure seems to be unrelated. |
Sorry, something went wrong.
mdickinson
left a comment
There was a problem hiding this comment.
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.
I did. Lets see how it's... |
Sorry, something went wrong.
mdickinson
left a comment
There was a problem hiding this comment.
LGTM. One suggestion for rewording the .. versionchanged note.
Sorry, something went wrong.
|
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.
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
https://bugs.python.org/issue44258