[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mercurial-devel
Subject:    Re: [PATCH 03 of 10] context: don't use mutable default argument value
From:       Augie Fackler <raf () durin42 ! com>
Date:       2016-12-28 23:13:20
Message-ID: 20161228231320.GA48276 () imladris ! local
[Download RAW message or body]

On Tue, Dec 27, 2016 at 11:03:03AM +0100, Pierre-Yves David wrote:
>
>
> On 12/27/2016 01:03 AM, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1482796356 25200
> > #      Mon Dec 26 16:52:36 2016 -0700
> > # Node ID 10645505563a162bb6d91a90e8b3ded769dcdbb1
> > # Parent  6d16de1b79686f5790764afeafb7cc2a4af0a1bb
> > context: don't use mutable default argument value
> >
> > diff --git a/mercurial/context.py b/mercurial/context.py
> > --- a/mercurial/context.py
> > +++ b/mercurial/context.py
> > @@ -289,10 +289,11 @@ class basectx(object):
> >          context.
> >          '''
> >          return subrepo.subrepo(self, path, allowwdir=True)
> >
> > -    def match(self, pats=[], include=None, exclude=None, default='glob',
> > +    def match(self, pats=None, include=None, exclude=None, default='glob',
> >                listsubrepos=False, badfn=None):
> > +        pats = pats or []
>
>
> Same as before, we should explicitly test for 'None' here. Otherwise the
> code will behave differently (reuse the list vs create a new one) when
> passing an empty list as argument.
>
> An alternative would be to use '()' if the code is readonly. That would
> prevent later mutability bug and acknowledge the parameters is readonly.

In this case, I think None with an explicit 'is None' check is better,
for two reasons:

1) `x is None` is ~4x faster than `bool(x)` when `x` is any of `[]`,
   `()`, or `None`

2) Use of `None` as the sentinel for "unspecified" is in line with PEP
   484 type hints, so we can just use
   typing.Optional[typing.List[bytes]] instead of having to declare
   some richer sum type (assuming we ever get around to type
   annotations, which I hope we will.

Just my 2 ¢.

>
> (I don't often advocate for mixing tuple and list but this is one of the
> rare case)
>
> >          r = self._repo
> >          return matchmod.match(r.root, r.getcwd(), pats,
> >                                include, exclude, default,
> >                                auditor=r.nofsauditor, ctx=self,
> > @@ -1494,10 +1495,11 @@ class workingctx(committablectx):
> >                  elif self._repo.dirstate[dest] in 'r':
> >                      self._repo.dirstate.normallookup(dest)
> >                  self._repo.dirstate.copy(source, dest)
> >
> > -    def match(self, pats=[], include=None, exclude=None, default='glob',
> > +    def match(self, pats=None, include=None, exclude=None, default='glob',
> >                listsubrepos=False, badfn=None):
> > +        pats = pats or []
> >          r = self._repo
> >
> >          # Only a case insensitive filesystem needs magic to translate user input
> >          # to actual case in the filesystem.
> > diff --git a/tests/test-check-code.t b/tests/test-check-code.t
> > --- a/tests/test-check-code.t
> > +++ b/tests/test-check-code.t
> > @@ -39,14 +39,8 @@ New errors are not allowed. Warnings are
> >    Skipping i18n/polib.py it has no-che?k-code (glob)
> >    mercurial/changegroup.py:260:
> >     >               targetphase=phases.draft, expectedtotal=None):
> >     attribute default argument value may be mutable
> > -  mercurial/context.py:293:
> > -   >     def match(self, pats=[], include=None, exclude=None, default='glob',
> > -   mutable default argument value (list)
> > -  mercurial/context.py:1498:
> > -   >     def match(self, pats=[], include=None, exclude=None, default='glob',
> > -   mutable default argument value (list)
> >    mercurial/demandimport.py:309:
> >     >     if os.environ.get('HGDEMANDIMPORT') != 'disable':
> >     use encoding.environ instead (py3)
> >    mercurial/encoding.py:54:
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> 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

[prev in list] [next in list] [prev in thread] [next in thread]