Mailman 3 Re: [Python-Dev] cpython (3.1): #2650: Refactor re.escape to use enumerate(). - Python-Dev

newer
improvement to declaring abstract...

Re: [Python-Dev] cpython (3.1): #2650: Refactor re.escape to use enumerate().

older
GSoC idea: PEP process...

Georg Brandl

26 Mar 2011 26 Mar '11

12:17 p.m.

"Refactor" doesn't sound like it belongs in the 3.1 branch... Georg On 25.03.2011 13:27, ezio.melotti wrote:

...

http://hg.python.org/cpython/rev/ed02db9921ac changeset: 68924:ed02db9921ac branch: 3.1 user: Ezio Melotti date: Fri Mar 25 14:19:30 2011 +0200 summary: #2650: Refactor re.escape to use enumerate().

files: Lib/re.py | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/Lib/re.py b/Lib/re.py --- a/Lib/re.py +++ b/Lib/re.py @@ -223,8 +223,7 @@ if isinstance(pattern, str): alphanum = _alphanum_str s = list(pattern) - for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern): if c not in alphanum: if c == "\000": s[i] = "\\000"

_______________________________________________ Python-checkins mailing list Python-checkins@python.org http://mail.python.org/mailman/listinfo/python-checkins

Show replies by date

Terry Reedy

26 Mar 26 Mar

1 p.m.

New subject: cpython (3.1): #2650: Refactor re.escape to use enumerate().

On 3/26/2011 2:17 PM, Georg Brandl wrote:

...

"Refactor" doesn't sound like it belongs in the 3.1 branch...

...

...
- for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern):

I would call thin 'Replace obsolete idiom in' rather than 'Refactor'. So are you criticizing the replacement or the mislabeling? -- Terry Jan Reedy

Antoine Pitrou

1:08 p.m.

New subject: cpython (3.1): #2650: Refactor re.escape to use enumerate().

On Sat, 26 Mar 2011 15:00:29 -0400 Terry Reedy wrote:

...

On 3/26/2011 2:17 PM, Georg Brandl wrote:

...
"Refactor" doesn't sound like it belongs in the 3.1 branch...

...
...
- for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern):

I would call thin 'Replace obsolete idiom in' rather than 'Refactor'. So are you criticizing the replacement or the mislabeling?

I think the criticism is that such gratuitous replacements in bugfix branches are both a waste of time and a possible regression. The only good reason to do them is if you think it will improve further merges of bugfix patches. Regards Antoine.

"Martin v. Löwis"

1:19 p.m.

New subject: cpython (3.1): #2650: Refactor re.escape to use enumerate().

Am 26.03.2011 20:00, schrieb Terry Reedy:

...

On 3/26/2011 2:17 PM, Georg Brandl wrote:

...
"Refactor" doesn't sound like it belongs in the 3.1 branch...

...
...
- for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern):

I would call thin 'Replace obsolete idiom in' rather than 'Refactor'. So are you criticizing the replacement or the mislabeling?

No - I believe he is critizing that a stylistic change is done in a maintenance branch. It's not a bug fix, AFAICT, so it should not have been done. Regards, Martin P.S. I haven't looked into the specific context, but the diff alone may actually cause behavior changes, depending on what pattern exactly is.

Georg Brandl

4:03 p.m.

New subject: cpython (3.1): #2650: Refactor re.escape to use enumerate().

On 26.03.2011 20:19, "Martin v. Löwis" wrote:

...

Am 26.03.2011 20:00, schrieb Terry Reedy:

...
On 3/26/2011 2:17 PM, Georg Brandl wrote:

...
"Refactor" doesn't sound like it belongs in the 3.1 branch...

...
...
- for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern):

I would call thin 'Replace obsolete idiom in' rather than 'Refactor'. So are you criticizing the replacement or the mislabeling?

No - I believe he is critizing that a stylistic change is done in a maintenance branch. It's not a bug fix, AFAICT, so it should not have been done.

Exactly, and two changesets before that there was another commit "Refactor the tests for re.escape" that was by far larger than this one, and not as easily reviewed as this one. In the end, this kind of change in a bugfix branch has zero gain, but a nonzero risk of gratuitous breakage. If it is necessary for future fixes or ease of merging bugfixes, I'd at least expect a note of that in the commit message justifying the breach of policy for a barely-maintenance branch. Georg

Ezio Melotti

11:59 p.m.

New subject: cpython (3.1): #2650: Refactor re.escape to use enumerate().

On 27/03/2011 0.03, Georg Brandl wrote:

...

On 26.03.2011 20:19, "Martin v. Löwis" wrote:

...
Am 26.03.2011 20:00, schrieb Terry Reedy:

...
On 3/26/2011 2:17 PM, Georg Brandl wrote:

...
"Refactor" doesn't sound like it belongs in the 3.1 branch...

...
- for i in range(len(pattern)): - c = pattern[i] + for i, c in enumerate(pattern): I would call thin 'Replace obsolete idiom in' rather than 'Refactor'. So are you criticizing the replacement or the mislabeling? No - I believe he is critizing that a stylistic change is done in a maintenance branch. It's not a bug fix, AFAICT, so it should not have been done. Exactly, and two changesets before that there was another commit "Refactor the tests for re.escape" that was by far larger than this one, and not as easily reviewed as this one.

In the end, this kind of change in a bugfix branch has zero gain, but a nonzero risk of gratuitous breakage. If it is necessary for future fixes or ease of merging bugfixes, I'd at least expect a note of that in the commit message justifying the breach of policy for a barely-maintenance branch.

Hi, these commits are part of #2650[0]. First, I refactored the existing tests[1] and added a few more tests[2] to have better coverage. Tests are usually ported to maintenance branches as well (because they could uncover bugs and also make merging easier), so I started working on 3.1. Then I refactored the function[3], and since the refactoring was trivial and I had extensive tests to make sure that the behavior was unchanged I included the refactoring in 3.1 too. FWIW I've been porting most of the commits that I do on 3.2 on 3.1 too (i.e. I'm considering both of them maintenance branches), and merging 3 branches rather than 2 doesn't make much difference with mercurial. [0]: http://bugs.python.org/issue2650 [1]: http://hg.python.org/cpython/rev/1402c719b7cf [2]: http://hg.python.org/cpython/rev/9147f7ed75b3 [3]: http://hg.python.org/cpython/rev/ed02db9921ac Best Regards, Ezio Melotti

...

Georg

_______________________________________________ Python-Dev mailing list Python-Dev@python.org http://mail.python.org/mailman/listinfo/python-dev Unsubscribe: http://mail.python.org/mailman/options/python-dev/ezio.melotti%40gmail.com

4778

Age (days ago)

4779

Last active (days ago)

List overview

Download

5 comments

5 participants

participants (5)

"Martin v. Löwis"
Antoine Pitrou
Ezio Melotti
Georg Brandl
Terry Reedy

Re: [Python-Dev] cpython (3.1): #2650: Refactor re.escape to use enumerate().

Georg Brandl

Terry Reedy

Antoine Pitrou

"Martin v. Löwis"

Georg Brandl

Ezio Melotti

tags

participants (5)