Summary: | Make most scm.py tests pass in preparation for splitting them up | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, dpranke, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2011-05-01 17:53:37 PDT
Created attachment 91863 [details]
Patch
Committed r85446: <http://trac.webkit.org/changeset/85446> 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? (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. (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. (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. (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. :) (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. :) (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. IMHO, it's more valuable for us to align with common Python practice than to optimize our development effort by a few percent. (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). (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. |