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

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Aug. 7, 2016, 6:48 p.m.
Message ID <uvazcife2.wl%foozy@lares.dti.ne.jp>
Download mbox | patch
Permalink /patch/16179/
State Not Applicable
Headers show

Comments

Katsunori FUJIWARA - Aug. 7, 2016, 6:48 p.m.
At Sun, 7 Aug 2016 18:18:17 +0200,
Pierre-Yves David wrote:
> 
> 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.

For patch #1, please add "level=" for bare level value 1 as below:

================

For patch #2, please drop whole the 2nd diff hunk for demandimport.py
in this patch below:

================
@@ -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
================


> 
> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Aug. 7, 2016, 9:22 p.m.
On 08/07/2016 08:48 PM, FUJIWARA Katsunori wrote:
>
> At Sun, 7 Aug 2016 18:18:17 +0200,
> Pierre-Yves David wrote:
>>
>> 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.
>
> For patch #1, please add "level=" for bare level value 1 as below:
>
> ================
> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
> --- a/mercurial/demandimport.py
> +++ b/mercurial/demandimport.py
> @@ -117,7 +117,8 @@ class _demandmod(object):
>                  if '.' in p:
>                      h, t = p.split('.', 1)
>                  if getattr(mod, h, nothing) is nothing:
> -                    setattr(mod, h, _demandmod(p, mod.__dict__, mod.__dict__))
> +                    setattr(mod, h,
> +                            _demandmod(p, mod.__dict__, mod.__dict__, level=1))
>                  elif t:
>                      subload(getattr(mod, h), t)
>
> @@ -211,7 +212,8 @@ def _demandimport(name, globals=None, lo
>              for comp in modname.split('.')[1:]:
>                  if getattr(mod, comp, nothing) is nothing:
>                      setattr(mod, comp,
> -                            _demandmod(comp, mod.__dict__, mod.__dict__))
> +                            _demandmod(comp, mod.__dict__, mod.__dict__,
> +                                       level=1))
>                  mod = getattr(mod, comp)
>              return mod
>
> ================
>
> For patch #2, please drop whole the 2nd diff hunk for demandimport.py
> in this patch below:
>
> ================
> @@ -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
> ================

This is done. Thanks

Patch

================
diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -117,7 +117,8 @@  class _demandmod(object):
                 if '.' in p:
                     h, t = p.split('.', 1)
                 if getattr(mod, h, nothing) is nothing:
-                    setattr(mod, h, _demandmod(p, mod.__dict__, mod.__dict__))
+                    setattr(mod, h,
+                            _demandmod(p, mod.__dict__, mod.__dict__, level=1))
                 elif t:
                     subload(getattr(mod, h), t)
 
@@ -211,7 +212,8 @@  def _demandimport(name, globals=None, lo
             for comp in modname.split('.')[1:]:
                 if getattr(mod, comp, nothing) is nothing:
                     setattr(mod, comp,
-                            _demandmod(comp, mod.__dict__, mod.__dict__))
+                            _demandmod(comp, mod.__dict__, mod.__dict__,
+                                       level=1))
                 mod = getattr(mod, comp)
             return mod