| 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
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.
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
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?
> 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
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?
> 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
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.
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 >
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
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