Patchwork demandimport: honor absolute_import (issue4029)

login
register
mail settings
Submitter Jason R. Coombs
Date Sept. 7, 2013, 1:48 a.m.
Message ID <6e4a42dd60124b0bbe770f63743390e5@BLUPR06MB003.namprd06.prod.outlook.com>
Download mbox | patch
Permalink /patch/2399/
State Rejected
Headers show

Comments

Jason R. Coombs - Sept. 7, 2013, 1:48 a.m.
# HG changeset patch
# User Jason R. Coombs <jaraco@jaraco.com>
# Date 1378518380 14400
#      Fri Sep 06 21:46:20 2013 -0400
# Node ID 6b00771ea6ce22dc3dda8b6da063d3112bfcbce8
# Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
demandimport: honor absolute_import (issue4029)
Katsunori FUJIWARA - Sept. 7, 2013, 6:35 p.m.
I also posted the patch fixing this issue in another way.

    http://selenic.com/pipermail/mercurial-devel/2013-September/053405.html

IMHO, saving and reusing "level" of original import request seems to
be cheaper than checking existence of 'absolute_import' in "locals".

At Sat, 7 Sep 2013 01:48:02 +0000,
Jason R. Coombs wrote:
> 
> # HG changeset patch
> # User Jason R. Coombs <jaraco@jaraco.com>
> # Date 1378518380 14400
> #      Fri Sep 06 21:46:20 2013 -0400
> # Node ID 6b00771ea6ce22dc3dda8b6da063d3112bfcbce8
> # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> demandimport: honor absolute_import (issue4029)
> 
> diff -r 1d07bf106c2a -r 6b00771ea6ce mercurial/demandimport.py
> --- a/mercurial/demandimport.py Wed Sep 04 18:42:55 2013 -0700
> +++ b/mercurial/demandimport.py Fri Sep 06 21:46:20 2013 -0400
> @@ -25,6 +25,7 @@
>  '''
> 
>  import __builtin__
> +import __future__
>  _origimport = __import__
> 
>  nothing = object()
> @@ -55,7 +56,7 @@
>      def _load(self):
>          if not self._module:
>              head, globals, locals, after = self._data
> -            mod = _origimport(head, globals, locals)
> +            mod = _origimport(head, globals, locals, [], self._level())
>              # load submodules
>              def subload(mod, p):
>                  h, t = p, None
> @@ -74,6 +75,18 @@
>                  locals[head] = mod
>              object.__setattr__(self, "_module", mod)
> 
> +    def _level(self):
> +        """
> +        Determine the 'level' parameter to the __import__ function, based on
> +        self._data.
> +        """
> +        head, globals, locals, after = self._data
> +        abs_import = globals.get('absolute_import', None)
> +        if isinstance(abs_import, __future__._Feature):
> +            # absolute_import was indicated, so disallow relative imports
> +            return 0
> +        return -1
> +
>      def __repr__(self):
>          if self._module:
>              return "<proxied module '%s'>" % self._data[0]
> @@ -81,7 +94,7 @@
>      def __call__(self, *args, **kwargs):
>          raise TypeError("%s object is not callable" % repr(self))
>      def __getattribute__(self, attr):
> -        if attr in ('_data', '_extend', '_load', '_module'):
> +        if attr in ('_data', '_extend', '_load', '_module', '_level'):
>              return object.__getattribute__(self, attr)
>          self._load()
>          return getattr(self._module, attr)
> diff -r 1d07bf106c2a -r 6b00771ea6ce tests/test-demandimport-absolute.py
> --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/test-demandimport-absolute.py       Fri Sep 06 21:46:20 2013 -0400
> @@ -0,0 +1,46 @@
> +"test demand import when absolute_import is indicated"
> +
> +import os
> +import sys
> +import shutil
> +import textwrap
> +import unittest
> +
> +import silenttestrunner
> +
> +from mercurial import demandimport
> +
> +class TestAbsoluteImport(unittest.TestCase):
> +
> +    def setUp(self):
> +        os.mkdir('pkg')
> +        f = open('pkg/mod.py', 'w')
> +        f.write(textwrap.dedent("""
> +            from __future__ import absolute_import
> +            import os
> +            """).lstrip())
> +        f.close()
> +        f = open('pkg/__init__.py', 'w')
> +        f.close()
> +        f = open('pkg/os.py', 'w')
> +        f.write('''val = "this is not the module you're looking for"\n''')
> +        f.close()
> +        demandimport.enable()
> +
> +    def tearDown(self):
> +        shutil.rmtree('pkg')
> +        demandimport.disable()
> +
> +    def test_absolute_import(self):
> +        if sys.version_info < (2,5):
> +            print("Test only viable on Python 2.5 or later")
> +            return
> +        import pkg.mod
> +        # trigger the loading of the module
> +        pkg.mod.__name__
> +        assert pkg.mod.os.__name__ == 'os'
> +        assert 'devnull' in dir(pkg.mod.os)
> +        assert 'val' not in dir(pkg.mod.os)
> +
> +if __name__ == '__main__':
> +    silenttestrunner.main(__name__)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Jason R. Coombs - Sept. 7, 2013, 8:23 p.m.
I concur. Fujiwara's approach is superior. I hadn't considered that the level 
attribute could be made available to the demand_mod. Nice work.

I do believe the unit test provided still holds value (and also does further 
prove that Fujiwara's fix works). I would prefer to keep the unit test 
included in this patch because it tests the demandimport functionality more 
directly and without invoking other mercurial machinery (tests in isolation).

I withdraw this patch and I will create a separate patch request for just the 
unit test.

> -----Original Message-----
> From: FUJIWARA Katsunori [mailto:foozy@lares.dti.ne.jp]
> Sent: Saturday, 07 September, 2013 14:35
> To: Jason R. Coombs
> Cc: mercurial-devel@selenic.com
> Subject: Re: [PATCH] demandimport: honor absolute_import (issue4029)
>
> I also posted the patch fixing this issue in another way.
>
>     http://selenic.com/pipermail/mercurial-devel/2013-
> September/053405.html
>
> IMHO, saving and reusing "level" of original import request seems to be
> cheaper than checking existence of 'absolute_import' in "locals".
>
> At Sat, 7 Sep 2013 01:48:02 +0000,
> Jason R. Coombs wrote:
> >
> > # HG changeset patch
> > # User Jason R. Coombs <jaraco@jaraco.com> # Date 1378518380 14400
> > #      Fri Sep 06 21:46:20 2013 -0400
> > # Node ID 6b00771ea6ce22dc3dda8b6da063d3112bfcbce8
> > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > demandimport: honor absolute_import (issue4029)
> >
> > diff -r 1d07bf106c2a -r 6b00771ea6ce mercurial/demandimport.py
> > --- a/mercurial/demandimport.py Wed Sep 04 18:42:55 2013 -0700
> > +++ b/mercurial/demandimport.py Fri Sep 06 21:46:20 2013 -0400
> > @@ -25,6 +25,7 @@
> >  '''
> >
> >  import __builtin__
> > +import __future__
> >  _origimport = __import__
> >
> >  nothing = object()
> > @@ -55,7 +56,7 @@
> >      def _load(self):
> >          if not self._module:
> >              head, globals, locals, after = self._data
> > -            mod = _origimport(head, globals, locals)
> > +            mod = _origimport(head, globals, locals, [],
> > + self._level())
> >              # load submodules
> >              def subload(mod, p):
> >                  h, t = p, None
> > @@ -74,6 +75,18 @@
> >                  locals[head] = mod
> >              object.__setattr__(self, "_module", mod)
> >
> > +    def _level(self):
> > +        """
> > +        Determine the 'level' parameter to the __import__ function, based 
> > on
> > +        self._data.
> > +        """
> > +        head, globals, locals, after = self._data
> > +        abs_import = globals.get('absolute_import', None)
> > +        if isinstance(abs_import, __future__._Feature):
> > +            # absolute_import was indicated, so disallow relative imports
> > +            return 0
> > +        return -1
> > +
> >      def __repr__(self):
> >          if self._module:
> >              return "<proxied module '%s'>" % self._data[0] @@ -81,7
> > +94,7 @@
> >      def __call__(self, *args, **kwargs):
> >          raise TypeError("%s object is not callable" % repr(self))
> >      def __getattribute__(self, attr):
> > -        if attr in ('_data', '_extend', '_load', '_module'):
> > +        if attr in ('_data', '_extend', '_load', '_module', '_level'):
> >              return object.__getattribute__(self, attr)
> >          self._load()
> >          return getattr(self._module, attr) diff -r 1d07bf106c2a -r
> > 6b00771ea6ce tests/test-demandimport-absolute.py
> > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > +++ b/tests/test-demandimport-absolute.py       Fri Sep 06 21:46:20 2013 -
> 0400
> > @@ -0,0 +1,46 @@
> > +"test demand import when absolute_import is indicated"
> > +
> > +import os
> > +import sys
> > +import shutil
> > +import textwrap
> > +import unittest
> > +
> > +import silenttestrunner
> > +
> > +from mercurial import demandimport
> > +
> > +class TestAbsoluteImport(unittest.TestCase):
> > +
> > +    def setUp(self):
> > +        os.mkdir('pkg')
> > +        f = open('pkg/mod.py', 'w')
> > +        f.write(textwrap.dedent("""
> > +            from __future__ import absolute_import
> > +            import os
> > +            """).lstrip())
> > +        f.close()
> > +        f = open('pkg/__init__.py', 'w')
> > +        f.close()
> > +        f = open('pkg/os.py', 'w')
> > +        f.write('''val = "this is not the module you're looking 
> > for"\n''')
> > +        f.close()
> > +        demandimport.enable()
> > +
> > +    def tearDown(self):
> > +        shutil.rmtree('pkg')
> > +        demandimport.disable()
> > +
> > +    def test_absolute_import(self):
> > +        if sys.version_info < (2,5):
> > +            print("Test only viable on Python 2.5 or later")
> > +            return
> > +        import pkg.mod
> > +        # trigger the loading of the module
> > +        pkg.mod.__name__
> > +        assert pkg.mod.os.__name__ == 'os'
> > +        assert 'devnull' in dir(pkg.mod.os)
> > +        assert 'val' not in dir(pkg.mod.os)
> > +
> > +if __name__ == '__main__':
> > +    silenttestrunner.main(__name__)
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> >
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Jason R. Coombs - Sept. 7, 2013, 8:36 p.m.
I've followed up with
http://selenic.com/pipermail/mercurial-devel/2013-September/053411.html
(which just includes the unit test).

> -----Original Message-----
> From: Jason R. Coombs
> Sent: Saturday, 07 September, 2013 16:23
> To: 'FUJIWARA Katsunori'
> Cc: mercurial-devel@selenic.com
> Subject: RE: [PATCH] demandimport: honor absolute_import (issue4029)
> 
> I concur. Fujiwara's approach is superior. I hadn't considered that the
level
> attribute could be made available to the demand_mod. Nice work.
> 
> I do believe the unit test provided still holds value (and also does
further
> prove that Fujiwara's fix works). I would prefer to keep the unit test
> included in this patch because it tests the demandimport functionality
more
> directly and without invoking other mercurial machinery (tests in
isolation).
> 
> I withdraw this patch and I will create a separate patch request for just
the
> unit test.
> 
> > -----Original Message-----
> > From: FUJIWARA Katsunori [mailto:foozy@lares.dti.ne.jp]
> > Sent: Saturday, 07 September, 2013 14:35
> > To: Jason R. Coombs
> > Cc: mercurial-devel@selenic.com
> > Subject: Re: [PATCH] demandimport: honor absolute_import (issue4029)
> >
> > I also posted the patch fixing this issue in another way.
> >
> >     http://selenic.com/pipermail/mercurial-devel/2013-
> > September/053405.html
> >
> > IMHO, saving and reusing "level" of original import request seems to be
> > cheaper than checking existence of 'absolute_import' in "locals".
> >
> > At Sat, 7 Sep 2013 01:48:02 +0000,
> > Jason R. Coombs wrote:
> > >
> > > # HG changeset patch
> > > # User Jason R. Coombs <jaraco@jaraco.com> # Date 1378518380 14400
> > > #      Fri Sep 06 21:46:20 2013 -0400
> > > # Node ID 6b00771ea6ce22dc3dda8b6da063d3112bfcbce8
> > > # Parent  1d07bf106c2ad1c7ef5e257e754ca8d858bd04b0
> > > demandimport: honor absolute_import (issue4029)
> > >
> > > diff -r 1d07bf106c2a -r 6b00771ea6ce mercurial/demandimport.py
> > > --- a/mercurial/demandimport.py Wed Sep 04 18:42:55 2013 -0700
> > > +++ b/mercurial/demandimport.py Fri Sep 06 21:46:20 2013 -0400
> > > @@ -25,6 +25,7 @@
> > >  '''
> > >
> > >  import __builtin__
> > > +import __future__
> > >  _origimport = __import__
> > >
> > >  nothing = object()
> > > @@ -55,7 +56,7 @@
> > >      def _load(self):
> > >          if not self._module:
> > >              head, globals, locals, after = self._data
> > > -            mod = _origimport(head, globals, locals)
> > > +            mod = _origimport(head, globals, locals, [],
> > > + self._level())
> > >              # load submodules
> > >              def subload(mod, p):
> > >                  h, t = p, None
> > > @@ -74,6 +75,18 @@
> > >                  locals[head] = mod
> > >              object.__setattr__(self, "_module", mod)
> > >
> > > +    def _level(self):
> > > +        """
> > > +        Determine the 'level' parameter to the __import__ function,
based
> > > on
> > > +        self._data.
> > > +        """
> > > +        head, globals, locals, after = self._data
> > > +        abs_import = globals.get('absolute_import', None)
> > > +        if isinstance(abs_import, __future__._Feature):
> > > +            # absolute_import was indicated, so disallow relative
imports
> > > +            return 0
> > > +        return -1
> > > +
> > >      def __repr__(self):
> > >          if self._module:
> > >              return "<proxied module '%s'>" % self._data[0] @@ -81,7
> > > +94,7 @@
> > >      def __call__(self, *args, **kwargs):
> > >          raise TypeError("%s object is not callable" % repr(self))
> > >      def __getattribute__(self, attr):
> > > -        if attr in ('_data', '_extend', '_load', '_module'):
> > > +        if attr in ('_data', '_extend', '_load', '_module',
'_level'):
> > >              return object.__getattribute__(self, attr)
> > >          self._load()
> > >          return getattr(self._module, attr) diff -r 1d07bf106c2a -r
> > > 6b00771ea6ce tests/test-demandimport-absolute.py
> > > --- /dev/null   Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/tests/test-demandimport-absolute.py       Fri Sep 06 21:46:20
2013 -
> > 0400
> > > @@ -0,0 +1,46 @@
> > > +"test demand import when absolute_import is indicated"
> > > +
> > > +import os
> > > +import sys
> > > +import shutil
> > > +import textwrap
> > > +import unittest
> > > +
> > > +import silenttestrunner
> > > +
> > > +from mercurial import demandimport
> > > +
> > > +class TestAbsoluteImport(unittest.TestCase):
> > > +
> > > +    def setUp(self):
> > > +        os.mkdir('pkg')
> > > +        f = open('pkg/mod.py', 'w')
> > > +        f.write(textwrap.dedent("""
> > > +            from __future__ import absolute_import
> > > +            import os
> > > +            """).lstrip())
> > > +        f.close()
> > > +        f = open('pkg/__init__.py', 'w')
> > > +        f.close()
> > > +        f = open('pkg/os.py', 'w')
> > > +        f.write('''val = "this is not the module you're looking
> > > for"\n''')
> > > +        f.close()
> > > +        demandimport.enable()
> > > +
> > > +    def tearDown(self):
> > > +        shutil.rmtree('pkg')
> > > +        demandimport.disable()
> > > +
> > > +    def test_absolute_import(self):
> > > +        if sys.version_info < (2,5):
> > > +            print("Test only viable on Python 2.5 or later")
> > > +            return
> > > +        import pkg.mod
> > > +        # trigger the loading of the module
> > > +        pkg.mod.__name__
> > > +        assert pkg.mod.os.__name__ == 'os'
> > > +        assert 'devnull' in dir(pkg.mod.os)
> > > +        assert 'val' not in dir(pkg.mod.os)
> > > +
> > > +if __name__ == '__main__':
> > > +    silenttestrunner.main(__name__)
> > > _______________________________________________
> > > Mercurial-devel mailing list
> > > Mercurial-devel@selenic.com
> > > http://selenic.com/mailman/listinfo/mercurial-devel
> > >
> >
> > ----------------------------------------------------------------------
> > [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff -r 1d07bf106c2a -r 6b00771ea6ce mercurial/demandimport.py
--- a/mercurial/demandimport.py Wed Sep 04 18:42:55 2013 -0700
+++ b/mercurial/demandimport.py Fri Sep 06 21:46:20 2013 -0400
@@ -25,6 +25,7 @@ 
 '''

 import __builtin__
+import __future__
 _origimport = __import__

 nothing = object()
@@ -55,7 +56,7 @@ 
     def _load(self):
         if not self._module:
             head, globals, locals, after = self._data
-            mod = _origimport(head, globals, locals)
+            mod = _origimport(head, globals, locals, [], self._level())
             # load submodules
             def subload(mod, p):
                 h, t = p, None
@@ -74,6 +75,18 @@ 
                 locals[head] = mod
             object.__setattr__(self, "_module", mod)

+    def _level(self):
+        """
+        Determine the 'level' parameter to the __import__ function, based on
+        self._data.
+        """
+        head, globals, locals, after = self._data
+        abs_import = globals.get('absolute_import', None)
+        if isinstance(abs_import, __future__._Feature):
+            # absolute_import was indicated, so disallow relative imports
+            return 0
+        return -1
+
     def __repr__(self):
         if self._module:
             return "<proxied module '%s'>" % self._data[0]
@@ -81,7 +94,7 @@ 
     def __call__(self, *args, **kwargs):
         raise TypeError("%s object is not callable" % repr(self))
     def __getattribute__(self, attr):
-        if attr in ('_data', '_extend', '_load', '_module'):
+        if attr in ('_data', '_extend', '_load', '_module', '_level'):
             return object.__getattribute__(self, attr)
         self._load()
         return getattr(self._module, attr)
diff -r 1d07bf106c2a -r 6b00771ea6ce tests/test-demandimport-absolute.py
--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/tests/test-demandimport-absolute.py       Fri Sep 06 21:46:20 2013 -0400
@@ -0,0 +1,46 @@ 
+"test demand import when absolute_import is indicated"
+
+import os
+import sys
+import shutil
+import textwrap
+import unittest
+
+import silenttestrunner
+
+from mercurial import demandimport
+
+class TestAbsoluteImport(unittest.TestCase):
+
+    def setUp(self):
+        os.mkdir('pkg')
+        f = open('pkg/mod.py', 'w')
+        f.write(textwrap.dedent("""
+            from __future__ import absolute_import
+            import os
+            """).lstrip())
+        f.close()
+        f = open('pkg/__init__.py', 'w')
+        f.close()
+        f = open('pkg/os.py', 'w')
+        f.write('''val = "this is not the module you're looking for"\n''')
+        f.close()
+        demandimport.enable()
+
+    def tearDown(self):
+        shutil.rmtree('pkg')
+        demandimport.disable()
+
+    def test_absolute_import(self):
+        if sys.version_info < (2,5):
+            print("Test only viable on Python 2.5 or later")
+            return
+        import pkg.mod
+        # trigger the loading of the module
+        pkg.mod.__name__
+        assert pkg.mod.os.__name__ == 'os'
+        assert 'devnull' in dir(pkg.mod.os)
+        assert 'val' not in dir(pkg.mod.os)
+
+if __name__ == '__main__':
+    silenttestrunner.main(__name__)