[webkit-reviews] review denied: [Bug 41153] rebaseline-chromium-webkit-tests should add or remove files to local git repository : [Attachment 59732] patch v1; fix style violation

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied MORITA Hajime
<morrita at google.com>'s request for review:
Bug 41153: rebaseline-chromium-webkit-tests should add or remove files to local
git repository
https://bugs.webkit.org/show_bug.cgi?id=41153

Attachment 59732: patch v1; fix style violation
https://bugs.webkit.org/attachment.cgi?id=59732&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Looks great. Putting r- for some nicpicks.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py: 
 +  
I think we need this blank line:

"Separate top-level function and class definitions with two blank lines."

http://www.python.org/dev/peps/pep-0008/


WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
 +	def _add_directories_recursively(self, path):
I would say _add_parent_directories or something. "recursively" sounds you will
add a directory and its descendants.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:352
 +	    parent, here = os.path.split(path)
Cannot we just use os.path.dirname here?

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:349
 +	def _add_directories_recursively(self, path):
Also, I would write a docstring here.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:458
 +	def pset(self, pname, pvalue, path):
I would use propset as the name of this function.

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:462
 +	def pget(self, pname, path):
propget

WebKitTools/ChangeLog:11
 +	    - add "-U" and "-q" options to rebaseline_chromium_webkit_tests.py
This can be done in a separate change, but it's OK for now. Please consider
splitting your patch next time.

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:4
77
 +			svn_error = True
This variable should be renamed to scm_error?

WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:4
78
 +		    if not svn_error and isinstance(self._scm, scm.SVN):
I will comment on this separately.