Patchwork [2,of,2] demandimport: omit default value of "level" at construction of _demandmod

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Aug. 6, 2016, 1:28 p.m.
Message ID <6c99f07655feaf18d09e.1470490095@juju>
Download mbox | patch
Permalink /patch/16154/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - Aug. 6, 2016, 1:28 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1470489873 -32400
#      Sat Aug 06 22:24:33 2016 +0900
# Node ID 6c99f07655feaf18d09e1d5846c54fc8c8223ddc
# Parent  1881f4a509a31d88a5db583c6db8d186040eba94
demandimport: omit default value of "level" at construction of _demandmod

This makes construction of _demandmod require explicit level value,
and helps to avoid issues like issue5208 in the future.
Yuya Nishihara - Aug. 7, 2016, 12:01 p.m.
On Sat, 06 Aug 2016 22:28:15 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1470489873 -32400
> #      Sat Aug 06 22:24:33 2016 +0900
> # Node ID 6c99f07655feaf18d09e1d5846c54fc8c8223ddc
> # Parent  1881f4a509a31d88a5db583c6db8d186040eba94
> demandimport: omit default value of "level" at construction of _demandmod

The series already ninja queued.

> --- a/mercurial/demandimport.py
> +++ b/mercurial/demandimport.py
> @@ -64,8 +64,12 @@ def _hgextimport(importfunc, name, globa
>          return importfunc(hgextname, globals, *args, **kwargs)
>  
>  class _demandmod(object):
> -    """module demand-loader and proxy"""
> -    def __init__(self, name, globals, locals, level=level):
> +    """module demand-loader and proxy
> +
> +    Specify 1 as 'level' argument at construction, to import module
> +    relatively.
> +    """
> +    def __init__(self, name, globals, locals, level):
>          if '.' in name:
>              head, rest = name.split('.', 1)
>              after = [rest]

> @@ -194,10 +198,9 @@ def _demandimport(name, globals=None, lo
>              if symbol is nothing:
>                  mn = '%s.%s' % (mod.__name__, attr)
>                  if mn in ignore:
> -                    importfunc = _origimport
> +                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
>                  else:
> -                    importfunc = _demandmod
> -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
> +                    symbol = _demandmod(attr, mod.__dict__, locals, 1)

This change is okay, but unrelated?
Pierre-Yves David - Aug. 7, 2016, 2:07 p.m.
On 08/06/2016 03:28 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1470489873 -32400
> #      Sat Aug 06 22:24:33 2016 +0900
> # Node ID 6c99f07655feaf18d09e1d5846c54fc8c8223ddc
> # Parent  1881f4a509a31d88a5db583c6db8d186040eba94
> demandimport: omit default value of "level" at construction of _demandmod
>
> This makes construction of _demandmod require explicit level value,
> and helps to avoid issues like issue5208 in the future.

This looks correct to me. Pushed, thanks.

Cheers,
Pierre-Yves David - Aug. 7, 2016, 2:08 p.m.
On 08/07/2016 02:01 PM, Yuya Nishihara wrote:
> On Sat, 06 Aug 2016 22:28:15 +0900, FUJIWARA Katsunori wrote:
>> # HG changeset patch
>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> # Date 1470489873 -32400
>> #      Sat Aug 06 22:24:33 2016 +0900
>> # Node ID 6c99f07655feaf18d09e1d5846c54fc8c8223ddc
>> # Parent  1881f4a509a31d88a5db583c6db8d186040eba94
>> demandimport: omit default value of "level" at construction of _demandmod
>
> The series already ninja queued.

Yep, apparently my confirmation email failed to leave yesterday. Sorry 
about that.

Cheers
Katsunori FUJIWARA - Aug. 7, 2016, 2:59 p.m.
At Sun, 7 Aug 2016 21:01:06 +0900,
Yuya Nishihara wrote:
> 
> On Sat, 06 Aug 2016 22:28:15 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1470489873 -32400
> > #      Sat Aug 06 22:24:33 2016 +0900
> > # Node ID 6c99f07655feaf18d09e1d5846c54fc8c8223ddc
> > # Parent  1881f4a509a31d88a5db583c6db8d186040eba94
> > demandimport: omit default value of "level" at construction of _demandmod
> 
> The series already ninja queued.
> 
> > --- a/mercurial/demandimport.py
> > +++ b/mercurial/demandimport.py
> > @@ -64,8 +64,12 @@ def _hgextimport(importfunc, name, globa
> >          return importfunc(hgextname, globals, *args, **kwargs)
> >  
> >  class _demandmod(object):
> > -    """module demand-loader and proxy"""
> > -    def __init__(self, name, globals, locals, level=level):
> > +    """module demand-loader and proxy
> > +
> > +    Specify 1 as 'level' argument at construction, to import module
> > +    relatively.
> > +    """
> > +    def __init__(self, name, globals, locals, level):
> >          if '.' in name:
> >              head, rest = name.split('.', 1)
> >              after = [rest]
> 
> > @@ -194,10 +198,9 @@ def _demandimport(name, globals=None, lo
> >              if symbol is nothing:
> >                  mn = '%s.%s' % (mod.__name__, attr)
> >                  if mn in ignore:
> > -                    importfunc = _origimport
> > +                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
> >                  else:
> > -                    importfunc = _demandmod
> > -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
> > +                    symbol = _demandmod(attr, mod.__dict__, locals, 1)
> 
> This change is okay, but unrelated?
> 

_demandmod.__init__() requires no optional argument before "level",
but __import__() as originalimport() requires "fromlist" as below:

    __import__(name, globals={}, locals={}, fromlist=[], level=-1)

Therefore, just passing "1" as level argument instead of "level=1"
invokes original __import__() with "fromlist=1" :-<

    @@ -196,7 +196,7 @@ def _demandimport(name, globals=None, lo
                         importfunc = _origimport
                     else:
                         importfunc = _demandmod
    -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
    +                symbol = importfunc(attr, mod.__dict__, locals, 1)
                     setattr(mod, attr, symbol)
     
                 # Record the importing module references this symbol so we can


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - Aug. 7, 2016, 3:20 p.m.
On Sun, 07 Aug 2016 23:59:37 +0900, FUJIWARA Katsunori wrote:
> > >              if symbol is nothing:
> > >                  mn = '%s.%s' % (mod.__name__, attr)
> > >                  if mn in ignore:
> > > -                    importfunc = _origimport
> > > +                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
> > >                  else:
> > > -                    importfunc = _demandmod
> > > -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
> > > +                    symbol = _demandmod(attr, mod.__dict__, locals, 1)
> > 
> > This change is okay, but unrelated?
> > 
> 
> _demandmod.__init__() requires no optional argument before "level",
> but __import__() as originalimport() requires "fromlist" as below:
> 
>     __import__(name, globals={}, locals={}, fromlist=[], level=-1)
> 
> Therefore, just passing "1" as level argument instead of "level=1"
> invokes original __import__() with "fromlist=1" :-<
> 
>     @@ -196,7 +196,7 @@ def _demandimport(name, globals=None, lo
>                          importfunc = _origimport
>                      else:
>                          importfunc = _demandmod
>     -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
>     +                symbol = importfunc(attr, mod.__dict__, locals, 1)
>                      setattr(mod, attr, symbol)

Both functions can take level=1 as before. I don't strongly disagree with
this change, but IMHO, 'level=1' is clearer than bare '1'. (I would write
'/*level=*/ 1' in C.)
Katsunori FUJIWARA - Aug. 7, 2016, 3:42 p.m.
At Mon, 8 Aug 2016 00:20:35 +0900,
Yuya Nishihara wrote:
> 
> On Sun, 07 Aug 2016 23:59:37 +0900, FUJIWARA Katsunori wrote:
> > > >              if symbol is nothing:
> > > >                  mn = '%s.%s' % (mod.__name__, attr)
> > > >                  if mn in ignore:
> > > > -                    importfunc = _origimport
> > > > +                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
> > > >                  else:
> > > > -                    importfunc = _demandmod
> > > > -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
> > > > +                    symbol = _demandmod(attr, mod.__dict__, locals, 1)
> > > 
> > > This change is okay, but unrelated?
> > > 
> > 
> > _demandmod.__init__() requires no optional argument before "level",
> > but __import__() as originalimport() requires "fromlist" as below:
> > 
> >     __import__(name, globals={}, locals={}, fromlist=[], level=-1)
> > 
> > Therefore, just passing "1" as level argument instead of "level=1"
> > invokes original __import__() with "fromlist=1" :-<
> > 
> >     @@ -196,7 +196,7 @@ def _demandimport(name, globals=None, lo
> >                          importfunc = _origimport
> >                      else:
> >                          importfunc = _demandmod
> >     -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
> >     +                symbol = importfunc(attr, mod.__dict__, locals, 1)
> >                      setattr(mod, attr, symbol)
> 
> Both functions can take level=1 as before. I don't strongly disagree with
> this change, but IMHO, 'level=1' is clearer than bare '1'. (I would write
> '/*level=*/ 1' in C.)
> 

Ah, I focused on invoking _demandmod.__init__() with explicit "1" like
as previous patch in this series, but keeping "level=" is better for
readability as you described.

Should I post follow up patch to rework this chunk ?


----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Aug. 7, 2016, 4:18 p.m.
On 08/07/2016 05:42 PM, FUJIWARA Katsunori wrote:
> At Mon, 8 Aug 2016 00:20:35 +0900,
> Yuya Nishihara wrote:
>>
>> On Sun, 07 Aug 2016 23:59:37 +0900, FUJIWARA Katsunori wrote:
>>>>>              if symbol is nothing:
>>>>>                  mn = '%s.%s' % (mod.__name__, attr)
>>>>>                  if mn in ignore:
>>>>> -                    importfunc = _origimport
>>>>> +                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
>>>>>                  else:
>>>>> -                    importfunc = _demandmod
>>>>> -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
>>>>> +                    symbol = _demandmod(attr, mod.__dict__, locals, 1)
>>>>
>>>> This change is okay, but unrelated?
>>>>
>>>
>>> _demandmod.__init__() requires no optional argument before "level",
>>> but __import__() as originalimport() requires "fromlist" as below:
>>>
>>>     __import__(name, globals={}, locals={}, fromlist=[], level=-1)
>>>
>>> Therefore, just passing "1" as level argument instead of "level=1"
>>> invokes original __import__() with "fromlist=1" :-<
>>>
>>>     @@ -196,7 +196,7 @@ def _demandimport(name, globals=None, lo
>>>                          importfunc = _origimport
>>>                      else:
>>>                          importfunc = _demandmod
>>>     -                symbol = importfunc(attr, mod.__dict__, locals, level=1)
>>>     +                symbol = importfunc(attr, mod.__dict__, locals, 1)
>>>                      setattr(mod, attr, symbol)
>>
>> Both functions can take level=1 as before. I don't strongly disagree with
>> this change, but IMHO, 'level=1' is clearer than bare '1'. (I would write
>> '/*level=*/ 1' in C.)
>>
>
> Ah, I focused on invoking _demandmod.__init__() with explicit "1" like
> as previous patch in this series, but keeping "level=" is better for
> readability as you described.
>
> Should I post follow up patch to rework this chunk ?

There is very few changeset over it, if you tell me how to amend it, I 
would be happy to do so.

Patch

diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -64,8 +64,12 @@  def _hgextimport(importfunc, name, globa
         return importfunc(hgextname, globals, *args, **kwargs)
 
 class _demandmod(object):
-    """module demand-loader and proxy"""
-    def __init__(self, name, globals, locals, level=level):
+    """module demand-loader and proxy
+
+    Specify 1 as 'level' argument at construction, to import module
+    relatively.
+    """
+    def __init__(self, name, globals, locals, level):
         if '.' in name:
             head, rest = name.split('.', 1)
             after = [rest]
@@ -194,10 +198,9 @@  def _demandimport(name, globals=None, lo
             if symbol is nothing:
                 mn = '%s.%s' % (mod.__name__, attr)
                 if mn in ignore:
-                    importfunc = _origimport
+                    symbol = _origimport(attr, mod.__dict__, locals, level=1)
                 else:
-                    importfunc = _demandmod
-                symbol = importfunc(attr, mod.__dict__, locals, level=1)
+                    symbol = _demandmod(attr, mod.__dict__, locals, 1)
                 setattr(mod, attr, symbol)
 
             # Record the importing module references this symbol so we can