Patchwork util: kill Python 2.4 deque.remove hack

login
register
mail settings
Submitter Adrian Buehlmann
Date May 16, 2015, 8:51 a.m.
Message ID <3d14c1217117795e3b79.1431766292@kork>
Download mbox | patch
Permalink /patch/9118/
State Accepted
Headers show

Comments

Adrian Buehlmann - May 16, 2015, 8:51 a.m.
# HG changeset patch
# User Adrian Buehlmann <adrian@cadifra.com>
# Date 1431759801 -7200
# Node ID 3d14c1217117795e3b792369224ed7fa68e31123
# Parent  1ef96a3b8b89a896f2c9f3f977dbef8f45bb0e26
util: kill Python 2.4 deque.remove hack
Martin von Zweigbergk - May 16, 2015, 9:06 a.m.
Does a follow-up that replaces util.deque with collections.deque make
sense? I suppose the hack was the only reason for the alias...

On Sat, May 16, 2015, 01:52 Adrian Buehlmann <adrian@cadifra.com> wrote:

> # HG changeset patch
> # User Adrian Buehlmann <adrian@cadifra.com>
> # Date 1431759801 -7200
> # Node ID 3d14c1217117795e3b792369224ed7fa68e31123
> # Parent  1ef96a3b8b89a896f2c9f3f977dbef8f45bb0e26
> util: kill Python 2.4 deque.remove hack
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -334,17 +334,7 @@
>
>      return f
>
> -try:
> -    collections.deque.remove
> -    deque = collections.deque
> -except AttributeError:
> -    # python 2.4 lacks deque.remove
> -    class deque(collections.deque):
> -        def remove(self, val):
> -            for i, v in enumerate(self):
> -                if v == val:
> -                    del self[i]
> -                    break
> +deque = collections.deque
>
>  class sortdict(dict):
>      '''a simple sorted dictionary'''
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Adrian Buehlmann - May 16, 2015, 9:11 a.m.
On 2015-05-16 11:06, Martin von Zweigbergk wrote:
> Does a follow-up that replaces util.deque with collections.deque make
> sense? I suppose the hack was the only reason for the alias...

Feel free to do that.
Martin von Zweigbergk - May 16, 2015, 6:29 p.m.
On Sat, May 16, 2015 at 2:11 AM Adrian Buehlmann <adrian@cadifra.com> wrote:

> On 2015-05-16 11:06, Martin von Zweigbergk wrote:
> > Does a follow-up that replaces util.deque with collections.deque make
> > sense? I suppose the hack was the only reason for the alias...
>
> Feel free to do that.
>

Thanks, will do.
Martin von Zweigbergk - May 16, 2015, 6:33 p.m.
I've pushed this to the clowncopter, thanks. Will send my follow-up patch
once this patch is in Matt's repo.

On Sat, May 16, 2015 at 1:52 AM Adrian Buehlmann <adrian@cadifra.com> wrote:

> # HG changeset patch
> # User Adrian Buehlmann <adrian@cadifra.com>
> # Date 1431759801 -7200
> # Node ID 3d14c1217117795e3b792369224ed7fa68e31123
> # Parent  1ef96a3b8b89a896f2c9f3f977dbef8f45bb0e26
> util: kill Python 2.4 deque.remove hack
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -334,17 +334,7 @@
>
>      return f
>
> -try:
> -    collections.deque.remove
> -    deque = collections.deque
> -except AttributeError:
> -    # python 2.4 lacks deque.remove
> -    class deque(collections.deque):
> -        def remove(self, val):
> -            for i, v in enumerate(self):
> -                if v == val:
> -                    del self[i]
> -                    break
> +deque = collections.deque
>
>  class sortdict(dict):
>      '''a simple sorted dictionary'''
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 16, 2015, 6:38 p.m.
On May 16, 2015, at 2:33 PM, Martin von Zweigbergk <martinvonz@google.com> wrote:

> I've pushed this to the clowncopter, thanks. Will send my follow-up patch once this patch is in Matt's repo.

If you want, I’d be happy to review the removal of the name forward series and then you could clowncopterize it.
Pierre-Yves David - May 19, 2015, 5:56 a.m.
On 05/16/2015 02:06 AM, Martin von Zweigbergk wrote:
> Does a follow-up that replaces util.deque with collections.deque make
> sense? I suppose the hack was the only reason for the alias...

I feel we should be methodical about this. Dropping the check/util 
implementation without actually cleaning the code base is a free ticket 
to obscure technical debt.
Adrian Buehlmann - May 19, 2015, 6:13 a.m.
On 2015-05-19 07:56, Pierre-Yves David wrote:
> 
> 
> On 05/16/2015 02:06 AM, Martin von Zweigbergk wrote:
>> Does a follow-up that replaces util.deque with collections.deque make
>> sense? I suppose the hack was the only reason for the alias...
> 
> I feel we should be methodical about this. Dropping the check/util 
> implementation without actually cleaning the code base is a free ticket 
> to obscure technical debt.
> 

I also feel you should be methodical about this. Separating the action
in two patches was actually a good thing.

BTW, the code is actually quite sprinkled with technical debt already.
Cleaning it up incrementally is better than not making any progress at all.

BTW 2, there may have actually been good reasons for keeping the deque
alias a bit. After all, it was public.

I'm perfectly ok with not sending such patches any more though, if you
don't want them.
Adrian Buehlmann - May 19, 2015, 7:14 a.m.
On 2015-05-17 01:08, Martin von Zweigbergk wrote:
> What is it that makes it only "good enough"? Is the imports you are not
> quite happy with?

No. I'm fine with the imports. You even seem to have been removing one
which is no longer needed after removing a use of the removed alias.

I think I wrote "good enough" because I'm not able to verify the patch
completely without redoing the work you did. This is one of the sort of
patches where you can't say it is obviously correct or not. If you've
run the testsuite, then it is ok. I didn't even run the testsuite
myself, because I think I can trust you having done that.

(BTW, we seem to spend an amazing amount of time on such minor issues.)

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -334,17 +334,7 @@ 
 
     return f
 
-try:
-    collections.deque.remove
-    deque = collections.deque
-except AttributeError:
-    # python 2.4 lacks deque.remove
-    class deque(collections.deque):
-        def remove(self, val):
-            for i, v in enumerate(self):
-                if v == val:
-                    del self[i]
-                    break
+deque = collections.deque
 
 class sortdict(dict):
     '''a simple sorted dictionary'''