Patchwork [1,of,2] sortdict: add iteritems method

login
register
mail settings
Submitter Sean Farley
Date Nov. 6, 2014, 11:09 p.m.
Message ID <45fd23c47bb429c84e28.1415315394@1.0.0.127.in-addr.arpa>
Download mbox | patch
Permalink /patch/6623/
State Accepted
Commit 565f97e71ce3b8f9b4298f4242b8efdccc298e31
Delegated to: Augie Fackler
Headers show

Comments

Sean Farley - Nov. 6, 2014, 11:09 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1413475012 25200
#      Thu Oct 16 08:56:52 2014 -0700
# Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
# Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
sortdict: add iteritems method

Nothing really fancy, just adding this method to make sure when iteritems is
used that it is iterated in correct order.
Martin von Zweigbergk - Nov. 7, 2014, 5:27 a.m.
Why? For use in an extension or future patch? Or does this fix something?

On Thu Nov 06 2014 at 3:10:18 PM Sean Farley <sean.michael.farley@gmail.com>
wrote:

> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1413475012 25200
> #      Thu Oct 16 08:56:52 2014 -0700
> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> sortdict: add iteritems method
>
> Nothing really fancy, just adding this method to make sure when iteritems
> is
> used that it is iterated in correct order.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -367,10 +367,12 @@ class sortdict(dict):
>              pass
>      def keys(self):
>          return self._list
>      def iterkeys(self):
>          return self._list.__iter__()
> +    def iteritems(self):
> +        return self.items().__iter__()
>
>  class lrucachedict(object):
>      '''cache most recent gets from or sets to this dictionary'''
>      def __init__(self, maxsize):
>          self._cache = {}
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Sean Farley - Nov. 7, 2014, 5:30 a.m.
Martin von Zweigbergk writes:

> Why? For use in an extension or future patch? Or does this fix something?

Yes, for a future patch. I should have mentioned that in the commit
message. (can be fixed in flight for the willing queuer ... queue-ist?)

> On Thu Nov 06 2014 at 3:10:18 PM Sean Farley <sean.michael.farley@gmail.com>
> wrote:
>
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1413475012 25200
>> #      Thu Oct 16 08:56:52 2014 -0700
>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>> sortdict: add iteritems method
>>
>> Nothing really fancy, just adding this method to make sure when iteritems
>> is
>> used that it is iterated in correct order.
>>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>              pass
>>      def keys(self):
>>          return self._list
>>      def iterkeys(self):
>>          return self._list.__iter__()
>> +    def iteritems(self):
>> +        return self.items().__iter__()
>>
>>  class lrucachedict(object):
>>      '''cache most recent gets from or sets to this dictionary'''
>>      def __init__(self, maxsize):
>>          self._cache = {}
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@selenic.com
>> http://selenic.com/mailman/listinfo/mercurial-devel
>>
Didly - Nov. 7, 2014, 8:11 a.m.
On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
<sean.michael.farley@gmail.com> wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1413475012 25200
> #      Thu Oct 16 08:56:52 2014 -0700
> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> sortdict: add iteritems method
>
> Nothing really fancy, just adding this method to make sure when iteritems is
> used that it is iterated in correct order.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -367,10 +367,12 @@ class sortdict(dict):
>              pass
>      def keys(self):
>          return self._list
>      def iterkeys(self):
>          return self._list.__iter__()
> +    def iteritems(self):
> +        return self.items().__iter__()
>
>  class lrucachedict(object):
>      '''cache most recent gets from or sets to this dictionary'''
>      def __init__(self, maxsize):
>          self._cache = {}

Won't this code construct the items() list first, and then iterate
over it? If so, maybe it would be best to do it the other way around,
i.e. write a proper sortdict.iteritems method, using a for loop and
yield and then make the sortdict.items method return
list(self.iteritems()).

Of course this would not matter much unless the sortdict is big, but
then you would not need iteritems unless that were the case, right?

Cheers,

Angel
Sean Farley - Nov. 7, 2014, 4:05 p.m.
Didly writes:

> On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
> <sean.michael.farley@gmail.com> wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1413475012 25200
>> #      Thu Oct 16 08:56:52 2014 -0700
>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>> sortdict: add iteritems method
>>
>> Nothing really fancy, just adding this method to make sure when iteritems is
>> used that it is iterated in correct order.
>>
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>              pass
>>      def keys(self):
>>          return self._list
>>      def iterkeys(self):
>>          return self._list.__iter__()
>> +    def iteritems(self):
>> +        return self.items().__iter__()
>>
>>  class lrucachedict(object):
>>      '''cache most recent gets from or sets to this dictionary'''
>>      def __init__(self, maxsize):
>>          self._cache = {}
>
> Won't this code construct the items() list first, and then iterate
> over it? If so, maybe it would be best to do it the other way around,
> i.e. write a proper sortdict.iteritems method, using a for loop and
> yield and then make the sortdict.items method return
> list(self.iteritems()).
>
> Of course this would not matter much unless the sortdict is big, but
> then you would not need iteritems unless that were the case, right?

You are correct. My intention with this patch was that
sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
then (out of habit) calling iteritems() should return the keys in order.

Following the maxim, "make it work, profile, make it faster, in that
order", I figure that if this turns out to be a noticeable slowdown then
we can fix it later.
Pierre-Yves David - Nov. 7, 2014, 5:19 p.m.
On 11/07/2014 04:05 PM, Sean Farley wrote:
>
> Didly writes:
>
>> On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
>> <sean.michael.farley@gmail.com> wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>> # Date 1413475012 25200
>>> #      Thu Oct 16 08:56:52 2014 -0700
>>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>>> sortdict: add iteritems method
>>>
>>> Nothing really fancy, just adding this method to make sure when iteritems is
>>> used that it is iterated in correct order.
>>>
>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>> --- a/mercurial/util.py
>>> +++ b/mercurial/util.py
>>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>>               pass
>>>       def keys(self):
>>>           return self._list
>>>       def iterkeys(self):
>>>           return self._list.__iter__()
>>> +    def iteritems(self):
>>> +        return self.items().__iter__()
>>>
>>>   class lrucachedict(object):
>>>       '''cache most recent gets from or sets to this dictionary'''
>>>       def __init__(self, maxsize):
>>>           self._cache = {}
>>
>> Won't this code construct the items() list first, and then iterate
>> over it? If so, maybe it would be best to do it the other way around,
>> i.e. write a proper sortdict.iteritems method, using a for loop and
>> yield and then make the sortdict.items method return
>> list(self.iteritems()).
>>
>> Of course this would not matter much unless the sortdict is big, but
>> then you would not need iteritems unless that were the case, right?
>
> You are correct. My intention with this patch was that
> sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
> then (out of habit) calling iteritems() should return the keys in order.
>
> Following the maxim, "make it work, profile, make it faster, in that
> order", I figure that if this turns out to be a noticeable slowdown then
> we can fix it later.

Meh sending adding an iteritems that is actually not doing the one job 
iteritems is supposed to do is a not super appealing.

Making it properly looks trivial. Can you send a V2 with `iteritems` 
doing the right thing and explanation of why you need it in the description?
Augie Fackler - Nov. 9, 2014, 2:12 p.m.
On Fri, Nov 07, 2014 at 05:19:45PM +0000, Pierre-Yves David wrote:
>
>
> On 11/07/2014 04:05 PM, Sean Farley wrote:
> >
> >Didly writes:
> >
> >>On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
> >><sean.michael.farley@gmail.com> wrote:
> >>># HG changeset patch
> >>># User Sean Farley <sean.michael.farley@gmail.com>
> >>># Date 1413475012 25200
> >>>#      Thu Oct 16 08:56:52 2014 -0700
> >>># Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
> >>># Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
> >>>sortdict: add iteritems method
> >>>
> >>>Nothing really fancy, just adding this method to make sure when iteritems is
> >>>used that it is iterated in correct order.
> >>>
> >>>diff --git a/mercurial/util.py b/mercurial/util.py
> >>>--- a/mercurial/util.py
> >>>+++ b/mercurial/util.py
> >>>@@ -367,10 +367,12 @@ class sortdict(dict):
> >>>              pass
> >>>      def keys(self):
> >>>          return self._list
> >>>      def iterkeys(self):
> >>>          return self._list.__iter__()
> >>>+    def iteritems(self):
> >>>+        return self.items().__iter__()
> >>>
> >>>  class lrucachedict(object):
> >>>      '''cache most recent gets from or sets to this dictionary'''
> >>>      def __init__(self, maxsize):
> >>>          self._cache = {}
> >>
> >>Won't this code construct the items() list first, and then iterate
> >>over it? If so, maybe it would be best to do it the other way around,
> >>i.e. write a proper sortdict.iteritems method, using a for loop and
> >>yield and then make the sortdict.items method return
> >>list(self.iteritems()).
> >>
> >>Of course this would not matter much unless the sortdict is big, but
> >>then you would not need iteritems unless that were the case, right?
> >
> >You are correct. My intention with this patch was that
> >sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
> >then (out of habit) calling iteritems() should return the keys in order.
> >
> >Following the maxim, "make it work, profile, make it faster, in that
> >order", I figure that if this turns out to be a noticeable slowdown then
> >we can fix it later.
>
> Meh sending adding an iteritems that is actually not doing the one job
> iteritems is supposed to do is a not super appealing.
>
> Making it properly looks trivial. Can you send a V2 with `iteritems` doing
> the right thing and explanation of why you need it in the description?

I queued these and forgot to tell the list (e3be8027abe9), should I drop them?

>
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Nov. 9, 2014, 4:11 p.m.
On 11/09/2014 02:12 PM, Augie Fackler wrote:
> On Fri, Nov 07, 2014 at 05:19:45PM +0000, Pierre-Yves David wrote:
>>
>>
>> On 11/07/2014 04:05 PM, Sean Farley wrote:
>>>
>>> Didly writes:
>>>
>>>> On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
>>>> <sean.michael.farley@gmail.com> wrote:
>>>>> # HG changeset patch
>>>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>>>> # Date 1413475012 25200
>>>>> #      Thu Oct 16 08:56:52 2014 -0700
>>>>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>>>>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>>>>> sortdict: add iteritems method
>>>>>
>>>>> Nothing really fancy, just adding this method to make sure when iteritems is
>>>>> used that it is iterated in correct order.
>>>>>
>>>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>>>> --- a/mercurial/util.py
>>>>> +++ b/mercurial/util.py
>>>>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>>>>               pass
>>>>>       def keys(self):
>>>>>           return self._list
>>>>>       def iterkeys(self):
>>>>>           return self._list.__iter__()
>>>>> +    def iteritems(self):
>>>>> +        return self.items().__iter__()
>>>>>
>>>>>   class lrucachedict(object):
>>>>>       '''cache most recent gets from or sets to this dictionary'''
>>>>>       def __init__(self, maxsize):
>>>>>           self._cache = {}
>>>>
>>>> Won't this code construct the items() list first, and then iterate
>>>> over it? If so, maybe it would be best to do it the other way around,
>>>> i.e. write a proper sortdict.iteritems method, using a for loop and
>>>> yield and then make the sortdict.items method return
>>>> list(self.iteritems()).
>>>>
>>>> Of course this would not matter much unless the sortdict is big, but
>>>> then you would not need iteritems unless that were the case, right?
>>>
>>> You are correct. My intention with this patch was that
>>> sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
>>> then (out of habit) calling iteritems() should return the keys in order.
>>>
>>> Following the maxim, "make it work, profile, make it faster, in that
>>> order", I figure that if this turns out to be a noticeable slowdown then
>>> we can fix it later.
>>
>> Meh sending adding an iteritems that is actually not doing the one job
>> iteritems is supposed to do is a not super appealing.
>>
>> Making it properly looks trivial. Can you send a V2 with `iteritems` doing
>> the right thing and explanation of why you need it in the description?
>
> I queued these and forgot to tell the list (e3be8027abe9), should I drop them?

I would say so.
Sean Farley - Nov. 9, 2014, 8 p.m.
Pierre-Yves David writes:

> On 11/09/2014 02:12 PM, Augie Fackler wrote:
>> On Fri, Nov 07, 2014 at 05:19:45PM +0000, Pierre-Yves David wrote:
>>>
>>>
>>> On 11/07/2014 04:05 PM, Sean Farley wrote:
>>>>
>>>> Didly writes:
>>>>
>>>>> On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
>>>>> <sean.michael.farley@gmail.com> wrote:
>>>>>> # HG changeset patch
>>>>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>>>>> # Date 1413475012 25200
>>>>>> #      Thu Oct 16 08:56:52 2014 -0700
>>>>>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>>>>>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>>>>>> sortdict: add iteritems method
>>>>>>
>>>>>> Nothing really fancy, just adding this method to make sure when iteritems is
>>>>>> used that it is iterated in correct order.
>>>>>>
>>>>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>>>>> --- a/mercurial/util.py
>>>>>> +++ b/mercurial/util.py
>>>>>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>>>>>               pass
>>>>>>       def keys(self):
>>>>>>           return self._list
>>>>>>       def iterkeys(self):
>>>>>>           return self._list.__iter__()
>>>>>> +    def iteritems(self):
>>>>>> +        return self.items().__iter__()
>>>>>>
>>>>>>   class lrucachedict(object):
>>>>>>       '''cache most recent gets from or sets to this dictionary'''
>>>>>>       def __init__(self, maxsize):
>>>>>>           self._cache = {}
>>>>>
>>>>> Won't this code construct the items() list first, and then iterate
>>>>> over it? If so, maybe it would be best to do it the other way around,
>>>>> i.e. write a proper sortdict.iteritems method, using a for loop and
>>>>> yield and then make the sortdict.items method return
>>>>> list(self.iteritems()).
>>>>>
>>>>> Of course this would not matter much unless the sortdict is big, but
>>>>> then you would not need iteritems unless that were the case, right?
>>>>
>>>> You are correct. My intention with this patch was that
>>>> sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
>>>> then (out of habit) calling iteritems() should return the keys in order.
>>>>
>>>> Following the maxim, "make it work, profile, make it faster, in that
>>>> order", I figure that if this turns out to be a noticeable slowdown then
>>>> we can fix it later.
>>>
>>> Meh sending adding an iteritems that is actually not doing the one job
>>> iteritems is supposed to do is a not super appealing.
>>>
>>> Making it properly looks trivial. Can you send a V2 with `iteritems` doing
>>> the right thing and explanation of why you need it in the description?
>>
>> I queued these and forgot to tell the list (e3be8027abe9), should I drop them?
>
> I would say so.

Yeah, I'll send a V2.
Augie Fackler - Nov. 10, 2014, 2:40 a.m.
On Nov 9, 2014, at 3:00 PM, Sean Farley <sean.michael.farley@gmail.com> wrote:

> 
> Pierre-Yves David writes:
> 
>> On 11/09/2014 02:12 PM, Augie Fackler wrote:
>>> On Fri, Nov 07, 2014 at 05:19:45PM +0000, Pierre-Yves David wrote:
>>>> 
>>>> 
>>>> On 11/07/2014 04:05 PM, Sean Farley wrote:
>>>>> 
>>>>> Didly writes:
>>>>> 
>>>>>> On Fri, Nov 7, 2014 at 12:09 AM, Sean Farley
>>>>>> <sean.michael.farley@gmail.com> wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>>>>>> # Date 1413475012 25200
>>>>>>> #      Thu Oct 16 08:56:52 2014 -0700
>>>>>>> # Node ID 45fd23c47bb429c84e28a3303fb2a1097c5082e2
>>>>>>> # Parent  2d54aa5397cdb1c697673ba10b7618d5ac25c69e
>>>>>>> sortdict: add iteritems method
>>>>>>> 
>>>>>>> Nothing really fancy, just adding this method to make sure when iteritems is
>>>>>>> used that it is iterated in correct order.
>>>>>>> 
>>>>>>> diff --git a/mercurial/util.py b/mercurial/util.py
>>>>>>> --- a/mercurial/util.py
>>>>>>> +++ b/mercurial/util.py
>>>>>>> @@ -367,10 +367,12 @@ class sortdict(dict):
>>>>>>>              pass
>>>>>>>      def keys(self):
>>>>>>>          return self._list
>>>>>>>      def iterkeys(self):
>>>>>>>          return self._list.__iter__()
>>>>>>> +    def iteritems(self):
>>>>>>> +        return self.items().__iter__()
>>>>>>> 
>>>>>>>  class lrucachedict(object):
>>>>>>>      '''cache most recent gets from or sets to this dictionary'''
>>>>>>>      def __init__(self, maxsize):
>>>>>>>          self._cache = {}
>>>>>> 
>>>>>> Won't this code construct the items() list first, and then iterate
>>>>>> over it? If so, maybe it would be best to do it the other way around,
>>>>>> i.e. write a proper sortdict.iteritems method, using a for loop and
>>>>>> yield and then make the sortdict.items method return
>>>>>> list(self.iteritems()).
>>>>>> 
>>>>>> Of course this would not matter much unless the sortdict is big, but
>>>>>> then you would not need iteritems unless that were the case, right?
>>>>> 
>>>>> You are correct. My intention with this patch was that
>>>>> sortdict.iteritems() should Do The Right Thing™. Creating a sortdict and
>>>>> then (out of habit) calling iteritems() should return the keys in order.
>>>>> 
>>>>> Following the maxim, "make it work, profile, make it faster, in that
>>>>> order", I figure that if this turns out to be a noticeable slowdown then
>>>>> we can fix it later.
>>>> 
>>>> Meh sending adding an iteritems that is actually not doing the one job
>>>> iteritems is supposed to do is a not super appealing.
>>>> 
>>>> Making it properly looks trivial. Can you send a V2 with `iteritems` doing
>>>> the right thing and explanation of why you need it in the description?
>>> 
>>> I queued these and forgot to tell the list (e3be8027abe9), should I drop them?
>> 
>> I would say so.
> 
> Yeah, I'll send a V2.

Dropped from crew. Sorry about the trouble.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -367,10 +367,12 @@  class sortdict(dict):
             pass
     def keys(self):
         return self._list
     def iterkeys(self):
         return self._list.__iter__()
+    def iteritems(self):
+        return self.items().__iter__()
 
 class lrucachedict(object):
     '''cache most recent gets from or sets to this dictionary'''
     def __init__(self, maxsize):
         self._cache = {}