Patchwork [5,of,7] children: use absolute_import

login
register
mail settings
Submitter Gregory Szorc
Date Feb. 10, 2016, 1:52 a.m.
Message ID <265d8d09b0be997064ea.1455069160@7.1.168.192.in-addr.arpa>
Download mbox | patch
Permalink /patch/13084/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Gregory Szorc - Feb. 10, 2016, 1:52 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1455068072 28800
#      Tue Feb 09 17:34:32 2016 -0800
# Node ID 265d8d09b0be997064ea4b5ec4e0ff5e50eadf88
# Parent  3f54d7079a3f6704c103633290aaca137411f4a2
children: use absolute_import
Martin von Zweigbergk - Feb. 10, 2016, 10:39 p.m.
On Tue, Feb 9, 2016 at 5:52 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1455068072 28800
> #      Tue Feb 09 17:34:32 2016 -0800
> # Node ID 265d8d09b0be997064ea4b5ec4e0ff5e50eadf88
> # Parent  3f54d7079a3f6704c103633290aaca137411f4a2
> children: use absolute_import
>
> diff --git a/hgext/children.py b/hgext/children.py
> --- a/hgext/children.py
> +++ b/hgext/children.py
> @@ -13,11 +13,17 @@
>  This extension is deprecated. You should use :hg:`log -r
>  "children(REV)"` instead.
>  '''
>
> -from mercurial import cmdutil
> -from mercurial.commands import templateopts
> +from __future__ import absolute_import
> +
>  from mercurial.i18n import _
> +from mercurial import (
> +    cmdutil,
> +    commands,
> +)
> +
> +templateopts = commands.templateopts

Does "from mercurial.commands import templateopts" not work ("from
mercurial.i18n import _" apparently works)? Maybe something gets
initialized by "from mercurial import commands" that we need? I don't
know much about how imports work.
Martijn Pieters - Feb. 12, 2016, 7:55 a.m.
On Wed, Feb 10, 2016 at 1:52 AM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
> +templateopts = commands.templateopts

Why the rebinding to a global here? Is there code that expects
`hgext.children.templateopts` to exist? That code should then be fixed
to use the reference from `mercurial.commands` directly.

If not, then why not just use ... `] + commands.templateopts,` in the
`@command(..)` decorator?
Martijn Pieters - Feb. 12, 2016, 7:55 a.m.
On Wed, Feb 10, 2016 at 10:39 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> Does "from mercurial.commands import templateopts" not work ("from
> mercurial.i18n import _" apparently works)? Maybe something gets
> initialized by "from mercurial import commands" that we need? I don't
> know much about how imports work.

No, this isn't about importing not working (it works just fine). But
to update the import style to import the module, then bind the name
`templateopts` as a global in this extension *anyway* is.. still
puzzling without a further explanation.
Gregory Szorc - Feb. 12, 2016, 8:15 a.m.
> On Feb 10, 2016, at 14:39, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
>> On Tue, Feb 9, 2016 at 5:52 PM, Gregory Szorc <gregory.szorc@gmail.com> wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1455068072 28800
>> #      Tue Feb 09 17:34:32 2016 -0800
>> # Node ID 265d8d09b0be997064ea4b5ec4e0ff5e50eadf88
>> # Parent  3f54d7079a3f6704c103633290aaca137411f4a2
>> children: use absolute_import
>> 
>> diff --git a/hgext/children.py b/hgext/children.py
>> --- a/hgext/children.py
>> +++ b/hgext/children.py
>> @@ -13,11 +13,17 @@
>> This extension is deprecated. You should use :hg:`log -r
>> "children(REV)"` instead.
>> '''
>> 
>> -from mercurial import cmdutil
>> -from mercurial.commands import templateopts
>> +from __future__ import absolute_import
>> +
>> from mercurial.i18n import _
>> +from mercurial import (
>> +    cmdutil,
>> +    commands,
>> +)
>> +
>> +templateopts = commands.templateopts
> 
> Does "from mercurial.commands import templateopts" not work ("from
> mercurial.i18n import _" apparently works)? Maybe something gets
> initialized by "from mercurial import commands" that we need? I don't
> know much about how imports work.

The import style convention only allows symbol imports from i18n and node. Everything else must do the re-aliasing.
Pierre-Yves David - Feb. 12, 2016, 3:51 p.m.
On 02/12/2016 07:55 AM, Martijn Pieters wrote:
> On Wed, Feb 10, 2016 at 10:39 PM, Martin von Zweigbergk
> <martinvonz@google.com> wrote:
>> Does "from mercurial.commands import templateopts" not work ("from
>> mercurial.i18n import _" apparently works)? Maybe something gets
>> initialized by "from mercurial import commands" that we need? I don't
>> know much about how imports work.
>
> No, this isn't about importing not working (it works just fine). But
> to update the import style to import the module, then bind the name
> `templateopts` as a global in this extension *anyway* is.. still
> puzzling without a further explanation.

While this is puzzling and I would be happy that someone investigate why 
children think it is worthy to not benefit from demandimport hereā€¦

This patch is not making anything worse and is valid. I've pushed it to 
the clowncopter.

(but, please someone investigate this templateopts thingy)

Patch

diff --git a/hgext/children.py b/hgext/children.py
--- a/hgext/children.py
+++ b/hgext/children.py
@@ -13,11 +13,17 @@ 
 This extension is deprecated. You should use :hg:`log -r
 "children(REV)"` instead.
 '''
 
-from mercurial import cmdutil
-from mercurial.commands import templateopts
+from __future__ import absolute_import
+
 from mercurial.i18n import _
+from mercurial import (
+    cmdutil,
+    commands,
+)
+
+templateopts = commands.templateopts
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
 # Note for extension authors: ONLY specify testedwith = 'internal' for
diff --git a/tests/test-check-py3-compat.t b/tests/test-check-py3-compat.t
--- a/tests/test-check-py3-compat.t
+++ b/tests/test-check-py3-compat.t
@@ -31,9 +31,8 @@ 
   doc/check-seclevel.py not using absolute_import
   doc/gendoc.py not using absolute_import
   doc/hgmanpage.py not using absolute_import
   hgext/__init__.py not using absolute_import
-  hgext/children.py not using absolute_import
   hgext/churn.py not using absolute_import
   hgext/clonebundles.py not using absolute_import
   hgext/color.py not using absolute_import
   hgext/convert/__init__.py not using absolute_import