59904 2011-05-01 17:53:37 -0700 Make most scm.py tests pass in preparation for splitting them up 2011-05-02 16:33:25 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 eric eric abarth dpranke tony oldest_to_newest 396151 0 eric 2011-05-01 17:53:37 -0700 Make most scm.py tests pass in preparation for splitting them up 396152 1 91863 eric 2011-05-01 17:54:39 -0700 Created attachment 91863 Patch 396155 2 eric 2011-05-01 17:58:33 -0700 Committed r85446: <http://trac.webkit.org/changeset/85446> 396602 3 91863 dpranke 2011-05-02 13:34:50 -0700 Comment on attachment 91863 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? 396652 4 abarth 2011-05-02 14:14:03 -0700 (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. 396683 5 eric 2011-05-02 14:45:12 -0700 (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. 396716 6 dpranke 2011-05-02 15:19:39 -0700 (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. 396726 7 eric 2011-05-02 15:38:06 -0700 (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. :) 396729 8 eric 2011-05-02 15:38:59 -0700 (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. :) 396737 9 dpranke 2011-05-02 15:49:14 -0700 (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. 396755 10 abarth 2011-05-02 16:07:22 -0700 IMHO, it's more valuable for us to align with common Python practice than to optimize our development effort by a few percent. 396775 11 eric 2011-05-02 16:31:24 -0700 (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). 396776 12 eric 2011-05-02 16:33:25 -0700 (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. 91863 2011-05-01 17:54:39 -0700 2011-05-02 13:34:50 -0700 Patch bug-59904-20110501175438.patch text/plain 3230 eric U3VidmVyc2lvbiBSZXZpc2lvbjogODU0MjcKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCBkNTBkNTI5YjQyNjQ2MDNlYWMxMjkzZTkwOGM3MDYyZmNh NjViNGUyLi5jNGUzNmMzZjhkMzMzMTI5MzJiZmVlY2MwZWVkMGI1ZGIyZGZlMjkwIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjAg QEAKIDIwMTEtMDUtMDEgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJraXQub3JnPgogCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1ha2UgbW9zdCBzY20ucHkg dGVzdHMgcGFzcyBpbiBwcmVwYXJhdGlvbiBmb3Igc3BsaXR0aW5nIHRoZW0gdXAKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU5OTA0CisKKyAgICAgICAg VGhlcmUgaXMgb25lIHJlbWFpbmluZyBmYWlsdXJlOgorICAgICAgICB3ZWJraXRweS5jb21tb24u Y2hlY2tvdXQuc2NtLnNjbV91bml0dGVzdC5HaXRUZXN0V2l0aE1vY2sudGVzdF9jcmVhdGVfcGF0 Y2gKKyAgICAgICAgd2hpY2ggaWRlbnRpZmllcyBhIGRpZmZlcmVuY2UgYmV0d2VlbiB0aGUgU1ZO IGFuZCBHaXQgdmVyc2lvbnMgb2YgdGhlCisgICAgICAgIFNDTSBhcGkuICBJJ20gbm90IGdvaW5n IHRvIGJvdGhlciBmaXhpbmcgdGhhdCB1bnRpbCB3ZSBzcGxpdCBvdXQgYWxsCisgICAgICAgIHRo ZSB0ZXN0cy4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3Nj bS9zY21fdW5pdHRlc3QucHk6CisKKzIwMTEtMDUtMDEgIEVyaWMgU2VpZGVsICA8ZXJpY0B3ZWJr aXQub3JnPgorCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGguCiAKICAgICAgICAgTW92 ZSBzY20ucHkgaW50byBpdHMgb3duIG1vZHVsZSBpbiBwcmVwYXJhdGlvbiBmb3Igc3BsaXR0aW5n IGl0IHVwCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291 dC9zY20vc2NtX3VuaXR0ZXN0LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hl Y2tvdXQvc2NtL3NjbV91bml0dGVzdC5weQppbmRleCBmMzQ3M2YxM2MwMzQwY2E4ZjcxOGI0ZDhh MWJlNjE2YTNiZGFmOTJkLi5mMGI0ODVjZmVmOGEyZWQ2MDdhOTAxODk5YjBkYmJhMTk5ZGVkOTE0 IDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20v c2NtX3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNr b3V0L3NjbS9zY21fdW5pdHRlc3QucHkKQEAgLTQ2LDcgKzQ2LDcgQEAgaW1wb3J0IHNodXRpbAog CiBmcm9tIGRhdGV0aW1lIGltcG9ydCBkYXRlCiBmcm9tIHdlYmtpdHB5LmNvbW1vbi5jaGVja291 dC5hcGkgaW1wb3J0IENoZWNrb3V0Ci1mcm9tIHdlYmtpdHB5LmNvbW1vbi5jaGVja291dC5zY20g aW1wb3J0IGRldGVjdF9zY21fc3lzdGVtLCBTQ00sIFNWTiwgR2l0LCBDaGVja291dE5lZWRzVXBk YXRlLCBjb21taXRfZXJyb3JfaGFuZGxlciwgQXV0aGVudGljYXRpb25FcnJvciwgQW1iaWd1b3Vz Q29tbWl0RXJyb3IsIGZpbmRfY2hlY2tvdXRfcm9vdCwgZGVmYXVsdF9zY20KK2Zyb20gLnNjbSBp bXBvcnQgZGV0ZWN0X3NjbV9zeXN0ZW0sIFNDTSwgU1ZOLCBHaXQsIENoZWNrb3V0TmVlZHNVcGRh dGUsIGNvbW1pdF9lcnJvcl9oYW5kbGVyLCBBdXRoZW50aWNhdGlvbkVycm9yLCBBbWJpZ3VvdXND b21taXRFcnJvciwgZmluZF9jaGVja291dF9yb290LCBkZWZhdWx0X3NjbQogZnJvbSB3ZWJraXRw eS5jb21tb24uY29uZmlnLmNvbW1pdHRlcnMgaW1wb3J0IENvbW1pdHRlciAgIyBGSVhNRTogVGhp cyBzaG91bGQgbm90IGJlIG5lZWRlZAogZnJvbSB3ZWJraXRweS5jb21tb24ubmV0LmJ1Z3ppbGxh IGltcG9ydCBBdHRhY2htZW50ICMgRklYTUU6IFRoaXMgc2hvdWxkIG5vdCBiZSBuZWVkZWQKIGZy b20gd2Via2l0cHkuY29tbW9uLnN5c3RlbS5leGVjdXRpdmUgaW1wb3J0IEV4ZWN1dGl2ZSwgcnVu X2NvbW1hbmQsIFNjcmlwdEVycm9yCkBAIC0yODUsNyArMjg1LDcgQEAgY2xhc3MgU0NNVGVzdCh1 bml0dGVzdC5UZXN0Q2FzZSk6CiAgICAgICAgIGF0dGFjaG1lbnQgPSBBdHRhY2htZW50KHsiYnVn X2lkIjogMTIzNDV9LCBOb25lKQogICAgICAgICBhdHRhY2htZW50LmNvbnRlbnRzID0gbGFtYmRh OiBwYXRjaF9jb250ZW50cwogCi0gICAgICAgIGpvZV9jb29sID0gQ29tbWl0dGVyKG5hbWU9Ikpv ZSBDb29sIiwgZW1haWxfb3JfZW1haWxzPU5vbmUpCisgICAgICAgIGpvZV9jb29sID0gQ29tbWl0 dGVyKCJKb2UgQ29vbCIsICJqb2VAY29vbC5jb20iKQogICAgICAgICBhdHRhY2htZW50LnJldmll d2VyID0gbGFtYmRhOiBqb2VfY29vbAogCiAgICAgICAgIHJldHVybiBhdHRhY2htZW50CkBAIC0x NDUwLDcgKzE0NTAsNyBAQCBjbGFzcyBHaXRUZXN0V2l0aE1vY2sodW5pdHRlc3QuVGVzdENhc2Up OgogICAgICAgICBleGVjdXRpdmUuc2hvdWxkX2xvZyA9IFRydWUKIAogICAgIGRlZiB0ZXN0X2Ny ZWF0ZV9wYXRjaChzZWxmKToKLSAgICAgICAgZXhwZWN0ZWRfc3RkZXJyID0gIk1PQ0sgcnVuX2Nv bW1hbmQ6IFsnZ2l0JywgJ21lcmdlLWJhc2UnLCB1J3JlZnMvcmVtb3Rlcy9vcmlnaW4vbWFzdGVy JywgJ0hFQUQnXVxuTU9DSyBydW5fY29tbWFuZDogWydnaXQnLCAnZGlmZicsICctLWJpbmFyeScs ICctLW5vLWV4dC1kaWZmJywgJy0tZnVsbC1pbmRleCcsICctTScsICdNT0NLIG91dHB1dCBvZiBj aGlsZCBwcm9jZXNzJywgJy0tJ11cbiIKKyAgICAgICAgZXhwZWN0ZWRfc3RkZXJyID0gIk1PQ0sg cnVuX2NvbW1hbmQ6IFsnZ2l0JywgJ21lcmdlLWJhc2UnLCB1J3JlZnMvcmVtb3Rlcy9vcmlnaW4v bWFzdGVyJywgJ0hFQUQnXVxuTU9DSyBydW5fY29tbWFuZDogWydnaXQnLCAnZGlmZicsICctLWJp bmFyeScsICctLW5vLWV4dC1kaWZmJywgJy0tZnVsbC1pbmRleCcsICctTScsICdNT0NLIG91dHB1 dCBvZiBjaGlsZCBwcm9jZXNzJywgJy0tJ11cbk1PQ0sgcnVuX2NvbW1hbmQ6IFsnZ2l0JywgJ2xv ZycsICctMjUnXVxuIgogICAgICAgICBPdXRwdXRDYXB0dXJlKCkuYXNzZXJ0X291dHB1dHMoc2Vs Ziwgc2VsZi5zY20uY3JlYXRlX3BhdGNoLCBrd2FyZ3M9eydjaGFuZ2VkX2ZpbGVzJzogTm9uZX0s IGV4cGVjdGVkX3N0ZGVycj1leHBlY3RlZF9zdGRlcnIpCiAKIAo=