| 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
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 >
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.
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?
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