Patchwork [1,of,6] sortdict: have update() accept either dict or iterable of key/value pairs

login
register
mail settings
Submitter Yuya Nishihara
Date March 8, 2015, 11:56 a.m.
Message ID <023d0f46318665e8d01f.1425815763@mimosa>
Download mbox | patch
Permalink /patch/7932/
State Accepted
Headers show

Comments

Yuya Nishihara - March 8, 2015, 11:56 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1424267633 -32400
#      Wed Feb 18 22:53:53 2015 +0900
# Node ID 023d0f46318665e8d01fe42fb58ac1726532b4c0
# Parent  62c4a963489d0ff8887b1e5d2c9458d1e3384536
sortdict: have update() accept either dict or iterable of key/value pairs

Future patches will make templater stores sorted dict in _hybrid object.
sortdict should be constructed from sorted list.
Ryan McElroy - March 9, 2015, 7:05 a.m.
On 3/8/2015 4:56 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1424267633 -32400
> #      Wed Feb 18 22:53:53 2015 +0900
> # Node ID 023d0f46318665e8d01fe42fb58ac1726532b4c0
> # Parent  62c4a963489d0ff8887b1e5d2c9458d1e3384536
> sortdict: have update() accept either dict or iterable of key/value pairs
I'm always skeptical about making APIs accept more things -- it makes 
the code harder to understand and change in the future. I'd prefer that 
all callers of this function be changed to pass key-value pair 
iterables. I understand that might be hard though (because typehints 
don't exist, for one thing), so I'll leave it up to those who know more 
to decide if you should listen to this objection of mine or not :-)
>
> Future patches will make templater stores sorted dict in _hybrid object.
> sortdict should be constructed from sorted list.
English grammar nit-picking (changes in bold): "Future patches will make 
*the *templater *store **a **sortdict *in *the *_hybrid object. *This 
*sortdict should be constructed from *a *sorted list."
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -359,8 +359,10 @@ class sortdict(dict):
>       def __iter__(self):
>           return self._list.__iter__()
>       def update(self, src):
> -        for k in src:
> -            self[k] = src[k]
> +        if isinstance(src, dict):
> +            src = src.iteritems()
> +        for k, v in src:
> +            self[k] = v
>       def clear(self):
>           dict.clear(self)
>           self._list = []
>
Contents of patch look fine, despite my objections to the premise.
Yuya Nishihara - March 9, 2015, 1:27 p.m.
On Mon, 9 Mar 2015 00:05:00 -0700, Ryan McElroy wrote:
> On 3/8/2015 4:56 AM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1424267633 -32400
> > #      Wed Feb 18 22:53:53 2015 +0900
> > # Node ID 023d0f46318665e8d01fe42fb58ac1726532b4c0
> > # Parent  62c4a963489d0ff8887b1e5d2c9458d1e3384536
> > sortdict: have update() accept either dict or iterable of key/value pairs
> I'm always skeptical about making APIs accept more things -- it makes 
> the code harder to understand and change in the future. I'd prefer that 
> all callers of this function be changed to pass key-value pair 
> iterables.

I agree with you in general, but Python dict accepts both. Our sortdict should
be the same.

> > Future patches will make templater stores sorted dict in _hybrid object.
> > sortdict should be constructed from sorted list.
> English grammar nit-picking (changes in bold): "Future patches will make 
> *the *templater *store **a **sortdict *in *the *_hybrid object. *This 
> *sortdict should be constructed from *a *sorted list."

Ah, thanks.
Angel Ezquerra - March 9, 2015, 10:24 p.m.
On Mon, Mar 9, 2015 at 2:27 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Mon, 9 Mar 2015 00:05:00 -0700, Ryan McElroy wrote:
>> On 3/8/2015 4:56 AM, Yuya Nishihara wrote:
>> > # HG changeset patch
>> > # User Yuya Nishihara <yuya@tcha.org>
>> > # Date 1424267633 -32400
>> > #      Wed Feb 18 22:53:53 2015 +0900
>> > # Node ID 023d0f46318665e8d01fe42fb58ac1726532b4c0
>> > # Parent  62c4a963489d0ff8887b1e5d2c9458d1e3384536
>> > sortdict: have update() accept either dict or iterable of key/value pairs
>> I'm always skeptical about making APIs accept more things -- it makes
>> the code harder to understand and change in the future. I'd prefer that
>> all callers of this function be changed to pass key-value pair
>> iterables.
>
> I agree with you in general, but Python dict accepts both. Our sortdict should
> be the same.

I agree with Yuya. The sortdict class is basically mercurial's 2.4
compatible OrderedDict. As a dict-like class it makes a lot of sense
for its update method to acceot the same sort of inputs as a regular
dict, that is dicts or an iterable of key, value pairs.

Angel
Ryan McElroy - March 9, 2015, 11:35 p.m.
On 3/9/2015 3:24 PM, Angel Ezquerra wrote:
>> >I agree with you in general, but Python dict accepts both. Our sortdict should
>> >be the same.
> I agree with Yuya. The sortdict class is basically mercurial's 2.4
> compatible OrderedDict. As a dict-like class it makes a lot of sense
> for its update method to acceot the same sort of inputs as a regular
> dict, that is dicts or an iterable of key, value pairs.

Y'all convinced me, I withdraw all objections.

~Ryan

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -359,8 +359,10 @@  class sortdict(dict):
     def __iter__(self):
         return self._list.__iter__()
     def update(self, src):
-        for k in src:
-            self[k] = src[k]
+        if isinstance(src, dict):
+            src = src.iteritems()
+        for k, v in src:
+            self[k] = v
     def clear(self):
         dict.clear(self)
         self._list = []