| Submitter | Pierre-Yves David |
|---|---|
| Date | March 16, 2017, 11:28 a.m. |
| Message ID | <6c6d8c0cc67de1ecd744.1489663685@nodosa.octopoid.net> |
| Download | mbox | patch |
| Permalink | /patch/19385/ |
| State | Accepted |
| Headers | show |
Comments
On Thu, Mar 16, 2017 at 4:28 AM, Pierre-Yves David < pierre-yves.david@ens-lyon.org> wrote: > # HG changeset patch > # User Pierre-Yves David <pierre-yves.david@ens-lyon.org> > # Date 1489615423 25200 > # Wed Mar 15 15:03:43 2017 -0700 > # Node ID 6c6d8c0cc67de1ecd744a30889419510aa2064d2 > # Parent 361bccce566afe250c8d258f770a908713b2d13f > # EXP-Topic mutable-default > # Available At https://www.mercurial-scm.org/ > repo/users/marmoute/mercurial/ > # hg pull https://www.mercurial-scm.org/ > repo/users/marmoute/mercurial/ -r 6c6d8c0cc67d > rebase: explicitly tests for None > > Changeset 361bccce566a removed the mutable default value, but did not > explicitly > tested for None. Such implicit checking can introduce semantic and > performance > issue. We move to an explicit check for None as recommended by PEP8: > > https://www.python.org/dev/peps/pep-0008/#programming-recommendations I generally disagree with this series on the grounds that it doesn't matter in these use cases. Yes, there are times where this matters for performance or because we're leveraging dynamic programming to pass in different types that can all be empty. These are not those times. Also, if others insist on the "is None" check, I prefer the inline form (`revf = revf if revf is not None else []`) because I don't like wasting lines on type validation... because Python. > > > diff --git a/hgext/rebase.py b/hgext/rebase.py > --- a/hgext/rebase.py > +++ b/hgext/rebase.py > @@ -725,7 +725,8 @@ def _definesets(ui, repo, destf=None, sr > destspace=None): > """use revisions argument to define destination and rebase set > """ > - revf = revf or [] > + if revf is None: > + revf = [] > > # destspace is here to work around issues with `hg pull --rebase` see > # issue5214 for details > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
On Thu, Mar 16, 2017 at 09:35:12AM -0700, Gregory Szorc wrote: > On Thu, Mar 16, 2017 at 4:28 AM, Pierre-Yves David < > pierre-yves.david@ens-lyon.org> wrote: > > > # HG changeset patch > > # User Pierre-Yves David <pierre-yves.david@ens-lyon.org> > > # Date 1489615423 25200 > > # Wed Mar 15 15:03:43 2017 -0700 > > # Node ID 6c6d8c0cc67de1ecd744a30889419510aa2064d2 > > # Parent 361bccce566afe250c8d258f770a908713b2d13f > > # EXP-Topic mutable-default > > # Available At https://www.mercurial-scm.org/ > > repo/users/marmoute/mercurial/ > > # hg pull https://www.mercurial-scm.org/ > > repo/users/marmoute/mercurial/ -r 6c6d8c0cc67d > > rebase: explicitly tests for None > > > > Changeset 361bccce566a removed the mutable default value, but did not > > explicitly > > tested for None. Such implicit checking can introduce semantic and > > performance > > issue. We move to an explicit check for None as recommended by PEP8: > > > > https://www.python.org/dev/peps/pep-0008/#programming-recommendations > > > I generally disagree with this series on the grounds that it doesn't matter > in these use cases. Yes, there are times where this matters for performance > or because we're leveraging dynamic programming to pass in different types > that can all be empty. These are not those times. > > Also, if others insist on the "is None" check, I prefer the inline form > (`revf = revf if revf is not None else []`) because I don't like wasting > lines on type validation... because Python. It's a minor change, but on the whole it's more idiomatic, so I'm in favor of it. Queued (I don't object to the one-line form, but given that this series is here and done, I'm too lazy to re-do all the work.) > > > > > > > > diff --git a/hgext/rebase.py b/hgext/rebase.py > > --- a/hgext/rebase.py > > +++ b/hgext/rebase.py > > @@ -725,7 +725,8 @@ def _definesets(ui, repo, destf=None, sr > > destspace=None): > > """use revisions argument to define destination and rebase set > > """ > > - revf = revf or [] > > + if revf is None: > > + revf = [] > > > > # destspace is here to work around issues with `hg pull --rebase` see > > # issue5214 for details > > _______________________________________________ > > Mercurial-devel mailing list > > Mercurial-devel@mercurial-scm.org > > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel > > > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Patch
diff --git a/hgext/rebase.py b/hgext/rebase.py --- a/hgext/rebase.py +++ b/hgext/rebase.py @@ -725,7 +725,8 @@ def _definesets(ui, repo, destf=None, sr destspace=None): """use revisions argument to define destination and rebase set """ - revf = revf or [] + if revf is None: + revf = [] # destspace is here to work around issues with `hg pull --rebase` see # issue5214 for details