[webkit-reviews] review denied: [Bug 61773] Write a tools library to manipulate XCode project files : [Attachment 95553] new patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 61773: Write a tools library to manipulate XCode project files
https://bugs.webkit.org/show_bug.cgi?id=61773

Attachment 95553: new patch
https://bugs.webkit.org/attachment.cgi?id=95553&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95553&action=review

This is a lot of code, and I've very happy you're doing this work.  The big
thing missing from this patch (aside from the nits below) is tests!  We greatly
prefer this code to be tested using Python unit tests so we can make changes to
it without breaking things.  You can see lot of examples of other tests in
webkitpy in the files named _unittest.py.  If you write a unit test in a file
with such a name, it will be automagically discovered and run as part of the
test-webkitpy testing harness.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:42
> +    """Class used for exceptions."""

This docstring is meaningless.	Of course its a class used for exceptions. 
It's a subclass of Exception.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:45
> +    def __init__(self, msg):
> +	   self._msg = msg

Sorry to keep poking you with style nits, but we prefer variables to be full
english works, like we do in regular WebKit code, so this should be _message.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:58
> +    def __init__(self, strval):

strval => string_value or just string

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:274
> +	   # could not resolve path, return non-empty sub-path together with
current group

Also like regular WebKit code, we prefer comments to be English sentences with
initial caps and a trailing period.

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:618
> +    def parse_project_file(self, filePath, project_name=''):

filePath => file_path

> Tools/Scripts/webkitpy/common/project/xcodeproj.py:627
> +	   with codecs.open(filePath, 'r', 'utf8', 'replace') as self._file:
> +	       self._content = self._file.read()

Please use the filesystem abstraction for interacting with the file system. 
(Also, we require compatibility with Python 2.5, so you can't use "with"
without importing it from the future.)