[Webkit-unassigned] [Bug 75824] check-webkit-style should warn about missing svn:mime-type for png files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=75824


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #133272|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #19 from Tony Chang <tony at chromium.org>  2012-03-22 11:13:46 PST ---
(From update of attachment 133272)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.