Skip to content

remove unnecessary quantifier in regex in __format__ for ip_address.py #16362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

remove unnecessary quantifier in regex in __format__ for ip_address.py #16362

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/ipaddress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ def __format__(self, fmt):
# From here on down, support for 'bnXx'

import re
fmt_re = '^(?P<alternate>#?)(?P<grouping>_?)(?P<fmt_base>[xbnX]){1}$'
fmt_re = '^(?P<alternate>#?)(?P<grouping>_?)(?P<fmt_base>[xbnX])$'
Copy link
Contributor

@aeros aeros Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The {1} at the end of the regex specifically makes it match only one instance, removing it will make this match all instances. I'm not sure if that's at all important within this context though, so it would make sense to check with those who added it in the first place.

From looking at the git blame, this was added in the commit f9c95a4, authored by @ewosborne and merged by @zware 13 days ago. Let's wait on feedback from @zware before proceeding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even without the {1} it will match only one instance.
for example we can check here https://regex101.com/r/xtB5yX/1 ( in fact in the explanation it says {1} is not needed )
removing the {1} does not have any effect on what will match.

Copy link
Contributor

@aeros aeros Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

even without the {1} it will match only one instance.

I believe you're correct, but it's always better to be on the safe side and double check with the core dev who added/approved of the previous version when making any code revisions, no matter how minor the changes are. Especially if the commit was made fairly recently and the core dev is currently active.

Either way though, a core developer still needs to approve and merge the PR before it can be added. IMO, the most well suited person for doing so would be @zware since he merged the PR that added this section. Alternatively, @serhiy-storchaka is the most active expert of the re module and has made several changes within ipaddress.

Edit: Also, thanks for the introduction to regex101. I've mostly used regexr in the past, but it's useful to know of a visual tool that supports the Python specific flavor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I was remembering incorrectly when I initially said:

The {1} at the end of the regex specifically makes it match only one instance, removing it will make this match all instances.

For re.match(), it only matches at the beginning of the string, not each line. For matching all occurences, re.findall() is used instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that makes sense. let us wait for others to review this PR.
yeah I use regex101 to understand the regex visually than parsing it in my mind. it is an amazing tool.

m = re.match(fmt_re, fmt)
if not m:
return super().__format__(fmt)
Expand Down