GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||
GeezerVison is showing github.com — bpo-44258: support PEP 515 for Fraction's initialization from string by skirpichev · Pull Request #26422 · python/cpython Conversation
|
| denom = m.group('denom') | ||
| if denom: | ||
| denominator = int(denom) | ||
| denominator = m.group('den') |
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
|
Artifact uploading failure seems to be unrelated. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
I did. Lets see how it's... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
|
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.