Patchwork [6,of,6] tests: check import cycles in hgext/**.py, too

login
register
mail settings
Submitter Katsunori FUJIWARA
Date May 13, 2015, 4:53 p.m.
Message ID <e4eba789328746bf2f4e.1431536039@juju>
Download mbox | patch
Permalink /patch/9050/
State Superseded
Commit f8b602fd4643f86336c373f12d0e9d012a617602
Headers show

Comments

Katsunori FUJIWARA - May 13, 2015, 4:53 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1431535750 -32400
#      Thu May 14 01:49:10 2015 +0900
# Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
# Parent  28a977036067ea7bb0af50218edbee9abe82331a
tests: check import cycles in hgext/**.py, too

It is important to realize existing cycles in hgext/**.py.
Pierre-Yves David - May 13, 2015, 11:43 p.m.
On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1431535750 -32400
> #      Thu May 14 01:49:10 2015 +0900
> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
> tests: check import cycles in hgext/**.py, too

This series looks good to me and I've pushed it to the clowncopter.
CCing Augie as I think he may have a stronger opinion than me.
Augie Fackler - May 14, 2015, 12:11 a.m.
On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>
>
> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
> ># HG changeset patch
> ># User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> ># Date 1431535750 -32400
> >#      Thu May 14 01:49:10 2015 +0900
> ># Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
> ># Parent  28a977036067ea7bb0af50218edbee9abe82331a
> >tests: check import cycles in hgext/**.py, too
>
> This series looks good to me and I've pushed it to the clowncopter.
> CCing Augie as I think he may have a stronger opinion than me.

Patches 1 and 6 are obviously correct, but I'm unclear on the
correctness of 3 and 4, and therefore am also unsure of the merits of
2 and 5.

>
>
> --
> Pierre-Yves David
Pierre-Yves David - May 14, 2015, 12:12 a.m.
On 05/13/2015 05:11 PM, Augie Fackler wrote:
> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>
>>
>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>> # Date 1431535750 -32400
>>> #      Thu May 14 01:49:10 2015 +0900
>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>> tests: check import cycles in hgext/**.py, too
>>
>> This series looks good to me and I've pushed it to the clowncopter.
>> CCing Augie as I think he may have a stronger opinion than me.
>
> Patches 1 and 6 are obviously correct, but I'm unclear on the
> correctness of 3 and 4, and therefore am also unsure of the merits of
> 2 and 5.

What kind of uncorrectness do you fear?
Augie Fackler - May 14, 2015, 12:15 a.m.
> On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 05/13/2015 05:11 PM, Augie Fackler wrote:
>> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>> 
>>> 
>>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>>> # HG changeset patch
>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>> # Date 1431535750 -32400
>>>> #      Thu May 14 01:49:10 2015 +0900
>>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>>> tests: check import cycles in hgext/**.py, too
>>> 
>>> This series looks good to me and I've pushed it to the clowncopter.
>>> CCing Augie as I think he may have a stronger opinion than me.
>> 
>> Patches 1 and 6 are obviously correct, but I'm unclear on the
>> correctness of 3 and 4, and therefore am also unsure of the merits of
>> 2 and 5.
> 
> What kind of uncorrectness do you fear?

Basically, shadowing builtin module names in Python 3 is a dangerous proposition. Right now we have some places where the import-checker says we mix local imports with upstream packages, and we're actually (correctly) getting all local modules, but that breaks in Python 3 unless we move to explicit relative imports everywhere. I thought I responded to patch 3 with my concerns.

I'm tired from travel at this point, but am happy to talk more tomorrow about this series.

AF

> 
> -- 
> Pierre-Yves David
Pierre-Yves David - May 14, 2015, 12:19 a.m.
On 05/13/2015 05:15 PM, Augie Fackler wrote:
>
>> On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 05/13/2015 05:11 PM, Augie Fackler wrote:
>>> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>>>
>>>>
>>>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>>>> # HG changeset patch
>>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>>> # Date 1431535750 -32400
>>>>> #      Thu May 14 01:49:10 2015 +0900
>>>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>>>> tests: check import cycles in hgext/**.py, too
>>>>
>>>> This series looks good to me and I've pushed it to the clowncopter.
>>>> CCing Augie as I think he may have a stronger opinion than me.
>>>
>>> Patches 1 and 6 are obviously correct, but I'm unclear on the
>>> correctness of 3 and 4, and therefore am also unsure of the merits of
>>> 2 and 5.
>>
>> What kind of uncorrectness do you fear?
>
> Basically, shadowing builtin module names in Python 3 is a dangerous proposition. Right now we have some places where the import-checker says we mix local imports with upstream packages, and we're actually (correctly) getting all local modules, but that breaks in Python 3 unless we move to explicit relative imports everywhere. I thought I responded to patch 3 with my concerns.

You reply to patch 3 was fairly vague.

> I'm tired from travel at this point, but am happy to talk more tomorrow about this series.

Do you want me to drop it from the clowncopter?
Augie Fackler - May 14, 2015, 12:20 a.m.
> On May 13, 2015, at 20:19, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 05/13/2015 05:15 PM, Augie Fackler wrote:
>> 
>>> On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>> 
>>> 
>>> 
>>> On 05/13/2015 05:11 PM, Augie Fackler wrote:
>>>> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>>>> 
>>>>> 
>>>>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>>>>> # HG changeset patch
>>>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>>>> # Date 1431535750 -32400
>>>>>> #      Thu May 14 01:49:10 2015 +0900
>>>>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>>>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>>>>> tests: check import cycles in hgext/**.py, too
>>>>> 
>>>>> This series looks good to me and I've pushed it to the clowncopter.
>>>>> CCing Augie as I think he may have a stronger opinion than me.
>>>> 
>>>> Patches 1 and 6 are obviously correct, but I'm unclear on the
>>>> correctness of 3 and 4, and therefore am also unsure of the merits of
>>>> 2 and 5.
>>> 
>>> What kind of uncorrectness do you fear?
>> 
>> Basically, shadowing builtin module names in Python 3 is a dangerous proposition. Right now we have some places where the import-checker says we mix local imports with upstream packages, and we're actually (correctly) getting all local modules, but that breaks in Python 3 unless we move to explicit relative imports everywhere. I thought I responded to patch 3 with my concerns.
> 
> You reply to patch 3 was fairly vague.
> 
>> I'm tired from travel at this point, but am happy to talk more tomorrow about this series.
> 
> Do you want me to drop it from the clowncopter?

That would be my preference, yes.

> 
> -- 
> Pierre-Yves David
Pierre-Yves David - May 14, 2015, 12:53 a.m.
On 05/13/2015 05:20 PM, Augie Fackler wrote:
>
>> On May 13, 2015, at 20:19, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>>
>> On 05/13/2015 05:15 PM, Augie Fackler wrote:
>>>
>>>> On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/13/2015 05:11 PM, Augie Fackler wrote:
>>>>> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>>>>>
>>>>>>
>>>>>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>>>>> # Date 1431535750 -32400
>>>>>>> #      Thu May 14 01:49:10 2015 +0900
>>>>>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>>>>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>>>>>> tests: check import cycles in hgext/**.py, too
>>>>>>
>>>>>> This series looks good to me and I've pushed it to the clowncopter.
>>>>>> CCing Augie as I think he may have a stronger opinion than me.
>>>>>
>>>>> Patches 1 and 6 are obviously correct, but I'm unclear on the
>>>>> correctness of 3 and 4, and therefore am also unsure of the merits of
>>>>> 2 and 5.
>>>>
>>>> What kind of uncorrectness do you fear?
>>>
>>> Basically, shadowing builtin module names in Python 3 is a dangerous proposition. Right now we have some places where the import-checker says we mix local imports with upstream packages, and we're actually (correctly) getting all local modules, but that breaks in Python 3 unless we move to explicit relative imports everywhere. I thought I responded to patch 3 with my concerns.
>>
>> You reply to patch 3 was fairly vague.
>>
>>> I'm tired from travel at this point, but am happy to talk more tomorrow about this series.
>>
>> Do you want me to drop it from the clowncopter?
>
> That would be my preference, yes.

I've made the last 4 secret on the clowncopter.
Augie Fackler - May 14, 2015, 12:55 a.m.
If patch 6 applies, I'm happy to retain it. It seemed obviously right to
check hgext.
On May 13, 2015 8:53 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

>
>
> On 05/13/2015 05:20 PM, Augie Fackler wrote:
>
>>
>>  On May 13, 2015, at 20:19, Pierre-Yves David <
>>> pierre-yves.david@ens-lyon.org> wrote:
>>>
>>>
>>>
>>> On 05/13/2015 05:15 PM, Augie Fackler wrote:
>>>
>>>>
>>>>  On May 13, 2015, at 20:12, Pierre-Yves David <
>>>>> pierre-yves.david@ens-lyon.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 05/13/2015 05:11 PM, Augie Fackler wrote:
>>>>>
>>>>>> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
>>>>>>>
>>>>>>>> # HG changeset patch
>>>>>>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>>>>>>> # Date 1431535750 -32400
>>>>>>>> #      Thu May 14 01:49:10 2015 +0900
>>>>>>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
>>>>>>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
>>>>>>>> tests: check import cycles in hgext/**.py, too
>>>>>>>>
>>>>>>>
>>>>>>> This series looks good to me and I've pushed it to the clowncopter.
>>>>>>> CCing Augie as I think he may have a stronger opinion than me.
>>>>>>>
>>>>>>
>>>>>> Patches 1 and 6 are obviously correct, but I'm unclear on the
>>>>>> correctness of 3 and 4, and therefore am also unsure of the merits of
>>>>>> 2 and 5.
>>>>>>
>>>>>
>>>>> What kind of uncorrectness do you fear?
>>>>>
>>>>
>>>> Basically, shadowing builtin module names in Python 3 is a dangerous
>>>> proposition. Right now we have some places where the import-checker says we
>>>> mix local imports with upstream packages, and we're actually (correctly)
>>>> getting all local modules, but that breaks in Python 3 unless we move to
>>>> explicit relative imports everywhere. I thought I responded to patch 3 with
>>>> my concerns.
>>>>
>>>
>>> You reply to patch 3 was fairly vague.
>>>
>>>  I'm tired from travel at this point, but am happy to talk more tomorrow
>>>> about this series.
>>>>
>>>
>>> Do you want me to drop it from the clowncopter?
>>>
>>
>> That would be my preference, yes.
>>
>
> I've made the last 4 secret on the clowncopter.
>
> --
> Pierre-Yves David
>
Katsunori FUJIWARA - May 14, 2015, 6:23 a.m.
At Wed, 13 May 2015 20:15:41 -0400,
Augie Fackler wrote:
> 
> 
> > On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > 
> > 
> > 
> > On 05/13/2015 05:11 PM, Augie Fackler wrote:
> >> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
> >>> 
> >>> 
> >>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
> >>>> # HG changeset patch
> >>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> >>>> # Date 1431535750 -32400
> >>>> #      Thu May 14 01:49:10 2015 +0900
> >>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
> >>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
> >>>> tests: check import cycles in hgext/**.py, too
> >>> 
> >>> This series looks good to me and I've pushed it to the clowncopter.
> >>> CCing Augie as I think he may have a stronger opinion than me.
> >> 
> >> Patches 1 and 6 are obviously correct, but I'm unclear on the
> >> correctness of 3 and 4, and therefore am also unsure of the merits of
> >> 2 and 5.
> > 
> > What kind of uncorrectness do you fear?
> 
> Basically, shadowing builtin module names in Python 3 is a dangerous
> proposition. Right now we have some places where the import-checker
> says we mix local imports with upstream packages, and we're actually
> (correctly) getting all local modules, but that breaks in Python 3
> unless we move to explicit relative imports everywhere. I thought I
> responded to patch 3 with my concerns.

Oh, I had misunderstood that just separating local ones from upstream
packages is safe enough. Sorry, I'm not well at Python 3.

Then, what about steps below ?

  1. drop patches for "detect mixing imports" from this series

     before:
       #1: add xargs like mode
       #2: loop to get list of locally defined	modules at first
       #3: change the logic of "verify_stdlib_on_own_line()"
       #4: change the logic of "checkmod()"
       #5: remove useless stdlib_modules and related code paths
       #6: check import cycles in hgext/**.py, too

    after:
       #1: add xargs like mode
       #2: loop to get list of locally defined	modules at first
       #4: change the logic of "checkmod()" (*)
       #6: check import cycles in hgext/**.py, too

    (*) #4 needs some changes to be applied correctly

    This can improve cycle detection, as a first step.

  2. use explicit relative import for locally defined one

     Even though this requires many many changes, now is the good time
     to forget compatibility for Python 2.4, isn't it ? :-)

  3. change the logic of "verify_stdlib_on_own_line()"

     I assume the logic like below:

       examine whether name in implicit relative importing matches
       against locally defined one or not, and show warning "this
       import is ambiguous (or dangerous ?)" if so.

     ("verify_fromimport_for_local_one()" or so ?)

     This should show no warning, because there is no standard library
     importing, of which collides against locally defined one in
     Mercurial, AFAIK.

  4. remove useless stdlib_modules and related code paths

     Now, list of standard Python library isn't needed.


> I'm tired from travel at this point, but am happy to talk more tomorrow about this series.
> 
> AF
> 
> > 
> > -- 
> > Pierre-Yves David
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - May 14, 2015, 7:24 a.m.
At Thu, 14 May 2015 15:23:41 +0900,
FUJIWARA Katsunori wrote:
> 
> At Wed, 13 May 2015 20:15:41 -0400,
> Augie Fackler wrote:
> > 
> > 
> > > On May 13, 2015, at 20:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > > 
> > > 
> > > 
> > > On 05/13/2015 05:11 PM, Augie Fackler wrote:
> > >> On Wed, May 13, 2015 at 04:43:43PM -0700, Pierre-Yves David wrote:
> > >>> 
> > >>> 
> > >>> On 05/13/2015 09:53 AM, FUJIWARA Katsunori wrote:
> > >>>> # HG changeset patch
> > >>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > >>>> # Date 1431535750 -32400
> > >>>> #      Thu May 14 01:49:10 2015 +0900
> > >>>> # Node ID e4eba789328746bf2f4e2a0498c0afb08b80ae34
> > >>>> # Parent  28a977036067ea7bb0af50218edbee9abe82331a
> > >>>> tests: check import cycles in hgext/**.py, too
> > >>> 
> > >>> This series looks good to me and I've pushed it to the clowncopter.
> > >>> CCing Augie as I think he may have a stronger opinion than me.
> > >> 
> > >> Patches 1 and 6 are obviously correct, but I'm unclear on the
> > >> correctness of 3 and 4, and therefore am also unsure of the merits of
> > >> 2 and 5.
> > > 
> > > What kind of uncorrectness do you fear?
> > 
> > Basically, shadowing builtin module names in Python 3 is a dangerous
> > proposition. Right now we have some places where the import-checker
> > says we mix local imports with upstream packages, and we're actually
> > (correctly) getting all local modules, but that breaks in Python 3
> > unless we move to explicit relative imports everywhere. I thought I
> > responded to patch 3 with my concerns.
> 
> Oh, I had misunderstood that just separating local ones from upstream
> packages is safe enough. Sorry, I'm not well at Python 3.
> 
> Then, what about steps below ?
> 
>   1. drop patches for "detect mixing imports" from this series
> 
>      before:
>        #1: add xargs like mode
>        #2: loop to get list of locally defined	modules at first
>        #3: change the logic of "verify_stdlib_on_own_line()"
>        #4: change the logic of "checkmod()"
>        #5: remove useless stdlib_modules and related code paths
>        #6: check import cycles in hgext/**.py, too
> 
>     after:
>        #1: add xargs like mode
>        #2: loop to get list of locally defined	modules at first
>        #4: change the logic of "checkmod()" (*)
>        #6: check import cycles in hgext/**.py, too
> 
>     (*) #4 needs some changes to be applied correctly
> 
>     This can improve cycle detection, as a first step.
> 
>   2. use explicit relative import for locally defined one
> 
>      Even though this requires many many changes, now is the good time
>      to forget compatibility for Python 2.4, isn't it ? :-)
> 
>   3. change the logic of "verify_stdlib_on_own_line()"
> 
>      I assume the logic like below:
> 
>        examine whether name in implicit relative importing matches
>        against locally defined one or not, and show warning "this
>        import is ambiguous (or dangerous ?)" if so.

In addition to it, "from" module should be also examined whether
leading "." is added to locally defined one or not ("ImportFrom.level
== 1" in "ast" view ?), shouldn't it ?  For example:

  bad:
    from locally_defined_one import xxxx, ...

  good:
    from .locally_defined_one import xxxx, ...


>      ("verify_fromimport_for_local_one()" or so ?)
> 
>      This should show no warning, because there is no standard library
>      importing, of which collides against locally defined one in
>      Mercurial, AFAIK.
> 
>   4. remove useless stdlib_modules and related code paths
> 
>      Now, list of standard Python library isn't needed.
> 
> 
> > I'm tired from travel at this point, but am happy to talk more tomorrow about this series.
> > 
> > AF
> > 
> > > 
> > > -- 
> > > Pierre-Yves David
> > 
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> 
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/tests/test-module-imports.t b/tests/test-module-imports.t
--- a/tests/test-module-imports.t
+++ b/tests/test-module-imports.t
@@ -19,6 +19,7 @@  here that we should still endeavor to fi
 hidden by deduplication algorithm in the cycle detector, so fixing
 these may expose other cycles.
 
-  $ hg locate 'mercurial/**.py' | sed 's-\\-/-g' | python "$import_checker" -
+  $ hg locate 'mercurial/**.py' 'hgext/**.py' | sed 's-\\-/-g' | python "$import_checker" -
   Import cycle: mercurial.cmdutil -> mercurial.context -> mercurial.subrepo -> mercurial.cmdutil
+  Import cycle: hgext.largefiles.basestore -> hgext.largefiles.localstore -> hgext.largefiles.basestore
   Import cycle: mercurial.commands -> mercurial.commandserver -> mercurial.dispatch -> mercurial.commands