Patchwork [3,of,5,RFC] policy: add helper to import cpy/pure module selectively

login
register
mail settings
Submitter Yuya Nishihara
Date Aug. 13, 2016, 10:15 a.m.
Message ID <8bb0a9223bce791bae9e.1471083325@mimosa>
Download mbox | patch
Permalink /patch/16270/
State Changes Requested
Headers show

Comments

Yuya Nishihara - Aug. 13, 2016, 10:15 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1470969017 -32400
#      Fri Aug 12 11:30:17 2016 +0900
# Node ID 8bb0a9223bce791bae9e6a07238d6b9722e5577a
# Parent  bda4422fc315b54356ba1949e5ba861dd2660e86
policy: add helper to import cpy/pure module selectively

Unfortunately, __import__() nor getattr() does not take bytes, and globals()
is a dict keyed by unicodes on Python 3. uimportvars() is a unicode API to
comply with their requirement. I really hated mixing bytes and unicodes, but
bytes version of importvars() would be useless.

Nit: KeyError would be raised for invalid HGMODULEPOLICY. What should we
do in that case?
Gregory Szorc - Aug. 14, 2016, 4:38 p.m.
On Sat, Aug 13, 2016 at 3:15 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1470969017 -32400
> #      Fri Aug 12 11:30:17 2016 +0900
> # Node ID 8bb0a9223bce791bae9e6a07238d6b9722e5577a
> # Parent  bda4422fc315b54356ba1949e5ba861dd2660e86
> policy: add helper to import cpy/pure module selectively
>
> Unfortunately, __import__() nor getattr() does not take bytes, and
> globals()
> is a dict keyed by unicodes on Python 3. uimportvars() is a unicode API to
> comply with their requirement. I really hated mixing bytes and unicodes,
> but
> bytes version of importvars() would be useless.
>
> Nit: KeyError would be raised for invalid HGMODULEPOLICY. What should we
> do in that case?
>

We have precedence for this in the `hg` script: we write an untranslated
abort message
to sys.stderr and call sys.exit. Ideally we would translate that message.
But without
a module load policy, that gets difficult. We would probably ideally change
the policy to
"pure" (since that will always work), load i18n.py, then do a normal abort.
But to do that
may require moving some policy loading code into __init__.py.


>
> diff --git a/mercurial/policy.py b/mercurial/policy.py
> --- a/mercurial/policy.py
> +++ b/mercurial/policy.py
> @@ -23,6 +23,14 @@ policy = 'c'
>  policynoc = ('cffi', 'cffi-allow', 'py')
>  policynocffi = ('c', 'py')
>
> +_upackagetable = {
> +    'c': [u'cpy'],
> +    'allow': [u'cpy', u'pure'],
> +    'cffi': [u'pure'],  # TODO: [u'cffi']
> +    'cffi-allow': [u'pure'],  # TODO: [u'cffi', u'pure']
> +    'py': [u'pure'],
> +}
> +
>  try:
>      from . import __modulepolicy__
>      policy = __modulepolicy__.modulepolicy
> @@ -43,3 +51,24 @@ if sys.version_info[0] >= 3:
>
>  # Environment variable can always force settings.
>  policy = os.environ.get('HGMODULEPOLICY', policy)
> +
> +def _uexportedvars(mod):
> +    """Build dict of public names to simulate 'from mod import *'"""
> +    modvars = vars(mod)
> +    if u'__all__' in modvars:
> +        allnames = modvars[u'__all__']
> +        return dict((k, v) for k, v in modvars.items() if k in allnames)
> +    return dict((k, v) for k, v in modvars.items() if not
> k.startswith(u'_'))
> +
> +def uimportvars(uname):
> +    """Import module according to policy and return exported vars"""
> +    upkgnames = _upackagetable[policy]
> +    for upkg in upkgnames:
> +        try:
> +            # pass globals() to resolve name relative to this module
> +            mod = __import__(u'%s.%s' % (upkg, uname), globals(), level=1)
> +            mod = getattr(mod, uname)
> +            return _uexportedvars(mod)
> +        except ImportError:
> +            if upkg == upkgnames[-1]:
> +                raise
>

The "u" prefixing feels a bit weird. Your commit message says the bytes
version of "importvars" would be useless. So why the prefix?
Yuya Nishihara - Aug. 15, 2016, 1:09 a.m.
On Sun, 14 Aug 2016 09:38:26 -0700, Gregory Szorc wrote:
> On Sat, Aug 13, 2016 at 3:15 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1470969017 -32400
> > #      Fri Aug 12 11:30:17 2016 +0900
> > # Node ID 8bb0a9223bce791bae9e6a07238d6b9722e5577a
> > # Parent  bda4422fc315b54356ba1949e5ba861dd2660e86
> > policy: add helper to import cpy/pure module selectively
> >
> > Unfortunately, __import__() nor getattr() does not take bytes, and
> > globals()
> > is a dict keyed by unicodes on Python 3. uimportvars() is a unicode API to
> > comply with their requirement. I really hated mixing bytes and unicodes,
> > but
> > bytes version of importvars() would be useless.
> >
> > Nit: KeyError would be raised for invalid HGMODULEPOLICY. What should we
> > do in that case?
> >
> 
> We have precedence for this in the `hg` script: we write an untranslated
> abort message
> to sys.stderr and call sys.exit. Ideally we would translate that message.
> But without
> a module load policy, that gets difficult. We would probably ideally change
> the policy to
> "pure" (since that will always work), load i18n.py, then do a normal abort.
> But to do that
> may require moving some policy loading code into __init__.py.

I don't care much about i18n stuff for early exceptions. I'll try to handle
invalid policy in "hg" script.

> > +def uimportvars(uname):
> > +    """Import module according to policy and return exported vars"""
> > +    upkgnames = _upackagetable[policy]
> > +    for upkg in upkgnames:
> > +        try:
> > +            # pass globals() to resolve name relative to this module
> > +            mod = __import__(u'%s.%s' % (upkg, uname), globals(), level=1)
> > +            mod = getattr(mod, uname)
> > +            return _uexportedvars(mod)
> > +        except ImportError:
> > +            if upkg == upkgnames[-1]:
> > +                raise
> 
> The "u" prefixing feels a bit weird. Your commit message says the bytes
> version of "importvars" would be useless. So why the prefix?

Callers have to pass u'' instead of '', which is very different from the
other Mercurial APIs. So the function should have some ugliness to signal it.

Patch

diff --git a/mercurial/policy.py b/mercurial/policy.py
--- a/mercurial/policy.py
+++ b/mercurial/policy.py
@@ -23,6 +23,14 @@  policy = 'c'
 policynoc = ('cffi', 'cffi-allow', 'py')
 policynocffi = ('c', 'py')
 
+_upackagetable = {
+    'c': [u'cpy'],
+    'allow': [u'cpy', u'pure'],
+    'cffi': [u'pure'],  # TODO: [u'cffi']
+    'cffi-allow': [u'pure'],  # TODO: [u'cffi', u'pure']
+    'py': [u'pure'],
+}
+
 try:
     from . import __modulepolicy__
     policy = __modulepolicy__.modulepolicy
@@ -43,3 +51,24 @@  if sys.version_info[0] >= 3:
 
 # Environment variable can always force settings.
 policy = os.environ.get('HGMODULEPOLICY', policy)
+
+def _uexportedvars(mod):
+    """Build dict of public names to simulate 'from mod import *'"""
+    modvars = vars(mod)
+    if u'__all__' in modvars:
+        allnames = modvars[u'__all__']
+        return dict((k, v) for k, v in modvars.items() if k in allnames)
+    return dict((k, v) for k, v in modvars.items() if not k.startswith(u'_'))
+
+def uimportvars(uname):
+    """Import module according to policy and return exported vars"""
+    upkgnames = _upackagetable[policy]
+    for upkg in upkgnames:
+        try:
+            # pass globals() to resolve name relative to this module
+            mod = __import__(u'%s.%s' % (upkg, uname), globals(), level=1)
+            mod = getattr(mod, uname)
+            return _uexportedvars(mod)
+        except ImportError:
+            if upkg == upkgnames[-1]:
+                raise