Skip to content

GH-123945: Update regex for parsing negative numbers that contain underscores#123970

Merged
hauntsaninja merged 14 commits into
python:mainfrom
savannahostrowski:gh-123945
Sep 17, 2024
Merged

GH-123945: Update regex for parsing negative numbers that contain underscores#123970
hauntsaninja merged 14 commits into
python:mainfrom
savannahostrowski:gh-123945

Conversation

@savannahostrowski

@savannahostrowski savannahostrowski commented Sep 11, 2024

Copy link
Copy Markdown
Member

This PR updates the regex for parsing numbers with argparse to account for negative numbers with underscores. I've also added some test cases to account for negative ints and floats, and numbers that may contain multiple underscores. This behaviour repros on versions as old as 3.9, possibly earlier.

@savannahostrowski savannahostrowski added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes stdlib Standard Library Python modules in the Lib/ directory labels Sep 11, 2024
Comment thread Lib/argparse.py Outdated
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, I'm not sure the tests are testing the right scenario. Unless I'm mistaken, I think they pass without the change.

@savannahostrowski

Copy link
Copy Markdown
Member Author

Whoops, that's my bad. I should have tested without the fix as well. I've updated the tests and verified that they do fail without the regex update.

I wasn't sure where to put these new tests, so please let me know if there's a better way to do this or a better spot to place them.

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick update!

Don't have strong opinions on the location of the test, but it is a little weird that only two of the preexisting methods in TestParseKnownArgs test parse_known_args 🤷 The most similar test cases seem to be earlier in the file and use the ParserTestCase infra

Comment thread Lib/test/test_argparse.py Outdated
Comment thread Lib/test/test_argparse.py Outdated

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Comment thread Lib/test/test_argparse.py Outdated
Comment thread Misc/NEWS.d/next/Library/2024-09-11-19-05-32.gh-issue-123945.jLwybB.rst Outdated
@hauntsaninja
hauntsaninja merged commit 14e5bdc into python:main Sep 17, 2024
@miss-islington-app

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 17, 2024
…in underscores (pythonGH-123970)

---------

(cherry picked from commit 14e5bdc)

Co-authored-by: Savannah Ostrowski <savannahostrowski@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@miss-islington-app

Copy link
Copy Markdown

Sorry, @savannahostrowski and @hauntsaninja, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 14e5bdceff45e6e789e1f838b96988946c75b0f4 3.12

@bedevere-app

bedevere-app Bot commented Sep 17, 2024

Copy link
Copy Markdown

GH-124158 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Sep 17, 2024
@bedevere-app

bedevere-app Bot commented Sep 17, 2024

Copy link
Copy Markdown

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Sep 17, 2024
Comment thread Lib/argparse.py

# determines whether an "option" looks like a negative number
self._negative_number_matcher = _re.compile(r'^-\d+$|^-\d*\.\d+$')
self._negative_number_matcher = _re.compile(r'^-\d[\d_]*(\.\d[\d_]*)?$')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It does not match -.5 which was matched by the old code.

savannahostrowski added a commit to savannahostrowski/cpython that referenced this pull request Sep 22, 2024
…in underscores (python#123970)

---------

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@savannahostrowski
savannahostrowski deleted the gh-123945 branch September 27, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stdlib Standard Library Python modules in the Lib/ directory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants