Patchwork [3,of,8] namespaces: add convenience method called multify

login
register
mail settings
Submitter Sean Farley
Date Dec. 15, 2014, 12:37 a.m.
Message ID <519aa5b4f74d5604b2d3.1418603871@laptop.local>
Download mbox | patch
Permalink /patch/7096/
State Accepted
Headers show

Comments

Sean Farley - Dec. 15, 2014, 12:37 a.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1418598280 28800
#      Sun Dec 14 15:04:40 2014 -0800
# Node ID 519aa5b4f74d5604b2d3d34c4e75b99d698b01b5
# Parent  98bc28635ee1999c778e5c98f55423147d316a9e
namespaces: add convenience method called multify
Martin von Zweigbergk - Dec. 15, 2014, 5:52 a.m.
I find a patch that just adds dead code quite puzzling. The first thing I
do when I see a patch like this is to open the next patch in the series and
search for the name there. In this case it seems like it gets used in 6/8.
I would definitely prefer it squashed into that patch so I could evaluate
its usefulness more easily. If other reviewers do the same thing (look
ahead for the call site), it seems clearly better to squash it, but I'm
also curious what other reviewers think.

On Sun Dec 14 2014 at 4:38:40 PM Sean Farley <sean.michael.farley@gmail.com>
wrote:

> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1418598280 28800
> #      Sun Dec 14 15:04:40 2014 -0800
> # Node ID 519aa5b4f74d5604b2d3d34c4e75b99d698b01b5
> # Parent  98bc28635ee1999c778e5c98f55423147d316a9e
> namespaces: add convenience method called multify
>
> diff --git a/mercurial/namespaces.py b/mercurial/namespaces.py
> --- a/mercurial/namespaces.py
> +++ b/mercurial/namespaces.py
> @@ -1,7 +1,16 @@
>  from mercurial import util
>
> +def multify(val):
> +    """
> +    a convenience method to return an empty list instead of None
> +    """
> +    if val is None:
> +        return []
> +    else:
> +        return [val]
> +
>  class namespaces(object):
>      """
>      provides an interface to register a generic many-to-many mapping
> between
>      some (namespaced) names and nodes. The goal here is to control the
>      pollution of jamming things into tags or bookmarks (in
> extension-land) and
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Sean Farley - Dec. 15, 2014, 7:57 a.m.
Martin von Zweigbergk writes:

> I find a patch that just adds dead code quite puzzling. The first thing I
> do when I see a patch like this is to open the next patch in the series and
> search for the name there. In this case it seems like it gets used in 6/8.
> I would definitely prefer it squashed into that patch so I could evaluate
> its usefulness more easily. If other reviewers do the same thing (look
> ahead for the call site), it seems clearly better to squash it, but I'm
> also curious what other reviewers think.

In this case, I made an error while rebasing too quickly in a flurry
with Ryan and Pierre-Yves; definitely sorry about that.

In answer to your other question, I find it infinitely easier to fold
two patches then to split apart a patch. I personally prefer to add a
method, then use it in the next patch, but could see your preference as
well.

I guess I'll be interested to see what others say as well.
Durham Goode - Dec. 15, 2014, 6:47 p.m.
On 12/14/14 4:37 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1418598280 28800
> #      Sun Dec 14 15:04:40 2014 -0800
> # Node ID 519aa5b4f74d5604b2d3d34c4e75b99d698b01b5
> # Parent  98bc28635ee1999c778e5c98f55423147d316a9e
> namespaces: add convenience method called multify
>
> diff --git a/mercurial/namespaces.py b/mercurial/namespaces.py
> --- a/mercurial/namespaces.py
> +++ b/mercurial/namespaces.py
> @@ -1,7 +1,16 @@
>   from mercurial import util
>   
> +def multify(val):
> +    """
> +    a convenience method to return an empty list instead of None
> +    """
>
Is multify a word?  I've never heard it before, so naming this multify 
is about as meaningful as naming it pooperdooper().  How about 
"makelist()" or something?
Pierre-Yves David - Dec. 15, 2014, 6:47 p.m.
On 12/15/2014 10:47 AM, Durham Goode wrote:
>
> On 12/14/14 4:37 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1418598280 28800
>> #      Sun Dec 14 15:04:40 2014 -0800
>> # Node ID 519aa5b4f74d5604b2d3d34c4e75b99d698b01b5
>> # Parent  98bc28635ee1999c778e5c98f55423147d316a9e
>> namespaces: add convenience method called multify
>>
>> diff --git a/mercurial/namespaces.py b/mercurial/namespaces.py
>> --- a/mercurial/namespaces.py
>> +++ b/mercurial/namespaces.py
>> @@ -1,7 +1,16 @@
>>   from mercurial import util
>> +def multify(val):
>> +    """
>> +    a convenience method to return an empty list instead of None
>> +    """
>>
> Is multify a word?  I've never heard it before, so naming this multify
> is about as meaningful as naming it pooperdooper().  How about
> "makelist()" or something?

It has been renamed to "tolist" in the series now on the clowncopter.

Patch

diff --git a/mercurial/namespaces.py b/mercurial/namespaces.py
--- a/mercurial/namespaces.py
+++ b/mercurial/namespaces.py
@@ -1,7 +1,16 @@ 
 from mercurial import util
 
+def multify(val):
+    """
+    a convenience method to return an empty list instead of None
+    """
+    if val is None:
+        return []
+    else:
+        return [val]
+
 class namespaces(object):
     """
     provides an interface to register a generic many-to-many mapping between
     some (namespaced) names and nodes. The goal here is to control the
     pollution of jamming things into tags or bookmarks (in extension-land) and