remove unnecessary quantifier in regex in __format__ for ip_address.py #16362
Conversation
| @@ -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])$' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the verification @serhiy-storchaka. |
|
@serhiy-storchaka that looks even better ! thanks 👍 |
|
I will close this PR since the other PR has subsumed the change @serhiy-storchaka @aeros167 ? |
Sounds good, @serhiy-storchaka should be able to add a "co-authored-by" note in the PR he opened that includes you. |
This is very small re factor and a very straight forward change. There is no need to create an issue for this or add an entry in the news since no behavior change is happening.