gh-114014: Update `fractions.Fraction()`'s rational parsing regex by thatbirdguythatuknownot · Pull Request #114015 · python/cpython

thatbirdguythatuknownot · 2024-01-13T06:02:26Z

The previous regex caused unintentional inconsistency in error messages due to the below:

(?P<decimal>d*|\d+(_\d+)*)
            ^

Which matches the character d instead of the intended digit set \d. Fix it by adding a backslash behind d.

(?P<decimal>\d*|\d+(_\d+)*)
            ^^

Issue: Inconsistency in fractions.Fraction() initialization error with invalid string #114014

Fixes pythongh-114014. `\d` can easily be mistyped as simply `d`. Add tests for it. ``` (?P<decimal>d*|\d+(_\d+)*) ^^ ```

mdickinson

Thank you for this PR! The fix and the new tests look good to me. There's a PEP8 nit in about comments in the fractions.py file. We also need a news entry - are you in a position to add that? (I can create the news entry if you prefer.)

bedevere-app · 2024-01-13T11:22:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mdickinson · 2024-01-13T11:27:03Z

Random observation: the subpattern \d*|\d+(_\d+)* is slightly inelegant, in that the only case that the first branch of the regex covers that isn't already covered by the second branch is the empty string. (\d+(_\d+)*)? might be slightly better, and involves less backtracking in corner cases involving a large number of digits followed by an underscore (and more digits). That said, I don't think it's worth changing this in this PR. The bugfix backport in particular should be as simple as possible and not risk introducing more errors, so I think the fix in this PR is the right way to go.

Make sure the comments in the regex are 2 spaces away from the content.

thatbirdguythatuknownot · 2024-01-13T11:31:50Z

The subpattern \d*|\d+(_\d+)* is slightly inelegant, in that the only case that the first branch of the regex covers that isn't already covered by the second branch is the empty string. (\d+(_\d+)*)? might be slightly better, and involves less backtracking in corner cases involving a large number of digits followed by an underscore (and more digits).

I considered that, but the num group was using the former pattern; I decided to make the decimal group pattern consistent with that, since it seemed to me that the only point of the issue is to fix the decimal pattern and not change anything else.

mdickinson · 2024-01-13T11:33:44Z

it seemed to me that the only point of the issue is to fix the denom pattern and not change anything else

Yep, agreed - it's good to be conservative with bug fixes, especially those that'll be backported to older Python versions.

thatbirdguythatuknownot · 2024-01-13T11:35:33Z

I have made the requested changes; please review again.

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

mdickinson

LGTM

miss-islington-app · 2024-01-13T12:02:43Z

Thanks @thatbirdguythatuknownot for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

…ex (pythonGH-114015) Fix a bug in the regex used for parsing a string input to the `fractions.Fraction` constructor. That bug led to an inconsistent exception message being given for some inputs. --------- (cherry picked from commit dd56b57) Co-authored-by: Crowthebird <78076854+thatbirdguythatuknownot@users.noreply.github.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

miss-islington-app · 2024-01-13T12:02:51Z

Sorry, @thatbirdguythatuknownot and @mdickinson, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker dd56b5748317c3d504d6a9660d9207620c547f5c 3.11

bedevere-app · 2024-01-13T12:02:52Z

GH-114023 is a backport of this pull request to the 3.12 branch.

…ex (python#114015) Fix a bug in the regex used for parsing a string input to the `fractions.Fraction` constructor. That bug led to an inconsistent exception message being given for some inputs. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

bedevere-app · 2024-01-13T12:36:39Z

GH-114025 is a backport of this pull request to the 3.11 branch.

mdickinson · 2024-01-13T12:37:22Z

I wasn't able to cajole cherry_picker into doing the backport for me for 3.11, so I've backported manually in #114025.

bedevere-app · 2024-01-13T13:40:08Z

GH-114025 is a backport of this pull request to the 3.11 branch.

…gex (GH-114015) (#114023) Fix a bug in the regex used for parsing a string input to the `fractions.Fraction` constructor. That bug led to an inconsistent exception message being given for some inputs. --------- (cherry picked from commit dd56b57) Co-authored-by: Crowthebird <78076854+thatbirdguythatuknownot@users.noreply.github.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Mark Dickinson <dickinsm@gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

…gex (GH-114015) (#114025) Fix a bug in the regex used for parsing a string input to the `fractions.Fraction` constructor. That bug led to an inconsistent exception message being given for some inputs. --------- Co-authored-by: Crowthebird <78076854+thatbirdguythatuknownot@users.noreply.github.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

…ex (python#114015) Fix a bug in the regex used for parsing a string input to the `fractions.Fraction` constructor. That bug led to an inconsistent exception message being given for some inputs. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

thatbirdguythatuknownot added 2 commits January 13, 2024 13:49

Update Fraction()'s rational parsing regex

14d29b1

Add tests for invalid strings containing d

4f550ea

Fixes pythongh-114014. `\d` can easily be mistyped as simply `d`. Add tests for it. ``` (?P<decimal>d*|\d+(_\d+)*) ^^ ```

bedevere-app Bot added the awaiting review label Jan 13, 2024

bedevere-app Bot mentioned this pull request Jan 13, 2024

Inconsistency in fractions.Fraction() initialization error with invalid string #114014

Closed

AlexWaygood requested a review from mdickinson January 13, 2024 08:18

mdickinson suggested changes Jan 13, 2024

View reviewed changes

Comment thread Lib/fractions.py Outdated

bedevere-app Bot added awaiting changes and removed awaiting review labels Jan 13, 2024

mdickinson added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 13, 2024

Fix PEP8 nit

35076f2

Make sure the comments in the regex are 2 spaces away from the content.

📜🤖 Added by blurb_it.

9be42a0

mdickinson reviewed Jan 13, 2024

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2024-01-13-11-34-29.gh-issue-114014.WRHifN.rst Outdated

Fix markup

7572189

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>

mdickinson approved these changes Jan 13, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting changes labels Jan 13, 2024

mdickinson merged commit dd56b57 into python:main Jan 13, 2024

bedevere-app Bot removed the awaiting merge label Jan 13, 2024

miss-islington-app Bot assigned mdickinson Jan 13, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 13, 2024

mdickinson mentioned this pull request Jan 13, 2024

[3.11] gh-114014: Update fractions.Fraction()'s rational parsing regex (GH-114015) #114025

Merged

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Jan 13, 2024

scoder added a commit to scoder/quicktions that referenced this pull request Nov 28, 2024

Update tests from python/cpython#114015

1483f49

Uh oh!

Uh oh!

Conversation

thatbirdguythatuknownot commented Jan 13, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-app Bot commented Jan 13, 2024

Uh oh!

mdickinson commented Jan 13, 2024

Uh oh!

thatbirdguythatuknownot commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdickinson commented Jan 13, 2024

Uh oh!

thatbirdguythatuknownot commented Jan 13, 2024

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app Bot commented Jan 13, 2024

Uh oh!

miss-islington-app Bot commented Jan 13, 2024

Uh oh!

bedevere-app Bot commented Jan 13, 2024

Uh oh!

bedevere-app Bot commented Jan 13, 2024

Uh oh!

mdickinson commented Jan 13, 2024

Uh oh!

bedevere-app Bot commented Jan 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thatbirdguythatuknownot commented Jan 13, 2024 •

edited by bedevere-app Bot

Loading

thatbirdguythatuknownot commented Jan 13, 2024 •

edited

Loading