[webkit-reviews] review denied: [Bug 75824] check-webkit-style should warn about missing svn:mime-type for png files : [Attachment 133272] proposed fix

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Balazs Ankes <bank at inf.u-szeged.hu>'s
request for review:
Bug 75824: check-webkit-style should warn about missing svn:mime-type for png
files
https://bugs.webkit.org/show_bug.cgi?id=75824

Attachment 133272: proposed fix
https://bugs.webkit.org/attachment.cgi?id=133272&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133272&action=review


> Tools/Scripts/webkitpy/style/checkers/png.py:69
> +		   match = re.search("^\s*.*enable-auto-props = (yes|no)",
line)
> +		   if (match and match.group(0)[0] != "e" and not
there_is_enable_line) or (match and match.group(0).endswith("no")):
> +		       errorstr_autoprop = "Have to enable auto props in the
subversion config file (" + config_file_path + ", line:" + str(linenumber + 1)
+ "). "

Can we just look for "^\s*enable-auto-props\s*=\s*yes"? If we don't find the
line, we give this error message. I think matching commented out lines is
making the logic unnecessarily complex.

> Tools/Scripts/webkitpy/style/checkers/png.py:76
> +		   match = re.search("^\s*.*\*\.png = svn:mime-type=image/png",
line)
> +		   if match and match.group(0)[0] != "*" and not
there_is_png_line:

Can we just search for "^\s*\*\.png\s*=\s*svn:mime-type=image/png"?  If we
don't find the line, we give the png error message.

> Tools/Scripts/webkitpy/style/checkers/png.py:96
> +	   if self._platform.startswith('linux') or
self._platform.startswith('darwin'):
> +	       config_file_path = self._fs.join(self._fs.expanduser("~") + "/",
".subversion/config")

Nit: You don't need the + "/", os.path.join adds the path separator for you.

> Tools/Scripts/webkitpy/style/checkers/png.py:98
> +	       config_file_path = self._fs.join(os.environ['APPDATA'] + "\\",
"Subversion\config")

Nit: You don't need the + "\\", os.path.join adds the path separator for you.

> Tools/Scripts/webkitpy/style/checkers/png_unittest.py:50
> +	   def mock_handle_style_error(line_number, category, confidence,
> +					message):

Nit: Indent is weird.  I would just not wrap here.