Bug 59904 – Make most scm.py tests pass in preparation for splitting them up

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Make most scm.py tests pass in preparation for splitting them up

[Eric Seidel (no email)]

Created attachment 91863 [details]
Patch

[Eric Seidel (no email)]

Committed r85446: <http://trac.webkit.org/changeset/85446>

[Dirk Pranke]

Comment on attachment 91863 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91863&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:49
> +from .scm import detect_scm_system, SCM, SVN, Git, CheckoutNeedsUpdate, commit_error_handler, AuthenticationError, AmbiguousCommitError, find_checkout_root, default_scm

Nit: this change doesn't seem like good style?

[Adam Barth]

(In reply to comment #3)
> (From update of attachment 91863 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91863&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:49
> > +from .scm import detect_scm_system, SCM, SVN, Git, CheckoutNeedsUpdate, commit_error_handler, AuthenticationError, AmbiguousCommitError, find_checkout_root, default_scm
> 
> Nit: this change doesn't seem like good style?

What's wrong with it?  The style bot didn't yell.

[Eric Seidel (no email)]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 91863 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91863&action=review
> > 
> > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:49
> > > +from .scm import detect_scm_system, SCM, SVN, Git, CheckoutNeedsUpdate, commit_error_handler, AuthenticationError, AmbiguousCommitError, find_checkout_root, default_scm
> > 
> > Nit: this change doesn't seem like good style?
> 
> What's wrong with it?  The style bot didn't yell.

Just because the bot didn't yell, doesn't make it right. :)

But I am intentionally moving away from full path imports for inter-module dependencies.  git.py, svn,py, etc. are essentially private to the scm module, so I don't think they should carry full paths.

I'm trying to make our modules have harder API boundaries (controlled by __init__.py) to make our code easier to understand.  Hopefully this is moving in a direction you agree with.

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 91863 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=91863&action=review
> > > 
> > > > Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py:49
> > > > +from .scm import detect_scm_system, SCM, SVN, Git, CheckoutNeedsUpdate, commit_error_handler, AuthenticationError, AmbiguousCommitError, find_checkout_root, default_scm
> > > 
> > > Nit: this change doesn't seem like good style?
> > 
> > What's wrong with it?  The style bot didn't yell.
> 
> Just because the bot didn't yell, doesn't make it right. :)
> 
> But I am intentionally moving away from full path imports for inter-module dependencies.  git.py, svn,py, etc. are essentially private to the scm module, so I don't think they should carry full paths.
> 
> I'm trying to make our modules have harder API boundaries (controlled by __init__.py) to make our code easier to understand.  Hopefully this is moving in a direction you agree with.

PEP-8 explicitly disagrees with you on this. The main rationale is that it makes it harder to grep through the tree to figure what is getting imported where:

" - Relative imports for intra-package imports are highly discouraged.
      Always use the absolute package path for all imports.
      Even now that PEP 328 [7] is fully implemented in Python 2.5,
      its style of explicit relative imports is actively discouraged;
      absolute imports are more portable and usually more readable."

Also, apparently relative imports are based off of the current name of the module, and when you run a module directly, the name is "__name__", not "scm.py"), so the Python tutorial recommends that you always use absolute imports (see section 6.4.2).

For what it's worth, I thought like you did until Tony beat it out of a me several months ago. I have since been gradually migrating all of the relative imports in the code I touch to absolute.

[Eric Seidel (no email)]

(In reply to comment #6)
> PEP-8 explicitly disagrees with you on this. The main rationale is that it makes it harder to grep through the tree to figure what is getting imported where:
> 
> " - Relative imports for intra-package imports are highly discouraged.
>       Always use the absolute package path for all imports.
>       Even now that PEP 328 [7] is fully implemented in Python 2.5,
>       its style of explicit relative imports is actively discouraged;
>       absolute imports are more portable and usually more readable."

OK.  That doesn't really apply to private module files.  I guess if we added _ to the beginning of all non-public module files it might make relative imports "safer" in pep-8's eyes.

> Also, apparently relative imports are based off of the current name of the module, and when you run a module directly, the name is "__name__", not "scm.py"), so the Python tutorial recommends that you always use absolute imports (see section 6.4.2).

I'm not sure I understand what i means to be relative to __name__.

> For what it's worth, I thought like you did until Tony beat it out of a me several months ago. I have since been gradually migrating all of the relative imports in the code I touch to absolute.

I'm happy to have all those relative imports I just added made absolute.  I still think that for private files in a module relative may make more sense, but this isn't something I'm particularly passionate about.

Maybe if/when I change the files to _svn.py the webkitpy.common.checkout.scm._svn.py imports in scm.__init__.py will look ugly enough that folks will want them relative again. :)

[Eric Seidel (no email)]

(In reply to comment #7)
> (In reply to comment #6)
> > PEP-8 explicitly disagrees with you on this. The main rationale is that it makes it harder to grep through the tree to figure what is getting imported where:
> > 
> > " - Relative imports for intra-package imports are highly discouraged.
> >       Always use the absolute package path for all imports.
> >       Even now that PEP 328 [7] is fully implemented in Python 2.5,
> >       its style of explicit relative imports is actively discouraged;
> >       absolute imports are more portable and usually more readable."
> 
> OK.  That doesn't really apply to private module files.  I guess if we added _ to the beginning of all non-public module files it might make relative imports "safer" in pep-8's eyes.

"safer" in that you should never need to grep for them since they're private and only imported by other files in that module. :)

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > PEP-8 explicitly disagrees with you on this. The main rationale is that it makes it harder to grep through the tree to figure what is getting imported where:
> > 
> > " - Relative imports for intra-package imports are highly discouraged.
> >       Always use the absolute package path for all imports.
> >       Even now that PEP 328 [7] is fully implemented in Python 2.5,
> >       its style of explicit relative imports is actively discouraged;
> >       absolute imports are more portable and usually more readable."
> 
> OK.  That doesn't really apply to private module files.  I guess if we added _ to the beginning of all non-public module files it might make relative imports "safer" in pep-8's eyes.
> 

Perhaps I'm not understanding you, but how are you reaching the conclusion that that doesn't apply to private module files? (As an aside, I'm not 100% sure I know what you mean by "private module file", but I assume you mean that a private module file is a file that is only intended to be imported by other files in the same package. Python has no true concept of "private".

At any rate, I am all for tightening up package definitions and cross-package dependencies.

[Adam Barth]

IMHO, it's more valuable for us to align with common Python practice than to optimize our development effort by a few percent.

[Eric Seidel (no email)]

(In reply to comment #9)
> Perhaps I'm not understanding you, but how are you reaching the conclusion that that doesn't apply to private module files? (As an aside, I'm not 100% sure I know what you mean by "private module file", but I assume you mean that a private module file is a file that is only intended to be imported by other files in the same package. Python has no true concept of "private".

Again, I'm a python noob here.... in many python packages we use, they seem to have a bunch of _foo.py files which I'm interpreting to be "private" to the module.  I suspect that files named with an initial __ might not even be possible to import from outside of the module (judging by how __ names are magled in python class names).

[Eric Seidel (no email)]

(In reply to comment #10)
> IMHO, it's more valuable for us to align with common Python practice than to optimize our development effort by a few percent.

In general I agree, which is part of how we ended up adopting PEP8.  I do think that we also try to align with the rest of WebKit in some respects too.

I think this issue is not a big one (note I don't think any of the participates are particularly passionate on this issue), but I think it's provided an interesting discussion... at least to me. :)

As I stated above, I'm in no way objecting to changing the imports all to be absolute, including the ones I made relative in this patch.