Patchwork [1,of,8] rebase: explicitly tests for None

login
register
mail settings
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

Pierre-Yves David - March 16, 2017, 11:28 a.m.
# 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
Gregory Szorc - March 16, 2017, 4:35 p.m.
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
>
Augie Fackler - March 16, 2017, 4:47 p.m.
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