[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 #132787|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #16 from Tony Chang <tony at chromium.org>  2012-03-20 10:01:18 PST ---
(From update of attachment 132787)
View in context: https://bugs.webkit.org/attachment.cgi?id=132787&action=review

> Tools/Scripts/webkitpy/style/checker.py:482
>      JSON = 3
> +    PNG = 9
>      PYTHON = 4
>      TEXT = 5
>      WATCHLIST = 6

Please re-number these.

> Tools/Scripts/webkitpy/style/checkers/png.py:29
> +import re
> +import sys
> +import os

Sort.

> Tools/Scripts/webkitpy/style/checkers/png.py:44
> +        self.file_path = file_path
> +        self.handle_style_error = handle_style_error
> +        self.fs = filesystem or FileSystem()
> +        self.det = scm or SCMDetector(self.fs, Executive()).detect_scm_system(self.fs.getcwd())

Member variables only used by the class should be prefixed with _.  I think that applies to all of these.  Also, self._detector or self._scm_detector instead of self.det.

> Tools/Scripts/webkitpy/style/checkers/png.py:53
> +        try:
> +            detection = self.det.display_name()
> +        except:

Do we need this try/except block?  When would this fail?

> Tools/Scripts/webkitpy/style/checkers/png.py:56
> +        if(detection == "git"):

Remove the ()s

> Tools/Scripts/webkitpy/style/checkers/png.py:59
> +                    config_file_path = self.fs.expanduser("~") + "/.subversion/config"

fs.join

> Tools/Scripts/webkitpy/style/checkers/png.py:67
> +                    config_file_path = os.environ['APPDATA'] + "\Subversion\config"

fs.join

> Tools/Scripts/webkitpy/style/checkers/png.py:72
> +                    config_file = self.fs.open_text_file_for_reading(config_file_path)
> +                except IOError:
> +                    errorstr = "There is no " + config_file_path
> +                    self.handle_style_error(0, 'image/png', 5, errorstr)
> +                    return

This is the same as lines 60-64.  We can pull it out of the if.

> Tools/Scripts/webkitpy/style/checkers/png.py:75
> +            for line in config_file.readlines():
> +                linenumber += 1

for linenumber, line in enumerate(config_file.readlines()):

> Tools/Scripts/webkitpy/style/checkers/png.py:77
> +                if match and str(match.group(0))[0] != "e":

str() is unnecessary because match.group(0) is already a string.

> Tools/Scripts/webkitpy/style/checkers/png.py:87
> +        if(detection == "svn"):

Remove the ()s and make this an elif.

> Tools/Scripts/webkitpy/style/checkers/png.py:89
> +            if(prop_get != "image/png"):

Remove the ()s.

> Tools/Scripts/webkitpy/style/checkers/png_unittest.py:57
> +        scm = MockSCMDetector('svn')

Can we add some test cases for git?  For example, a test case with each of the following:

enable-auto-props = yes

#enable-auto-props = yes

enable-auto-props = yes
#enable-auto-props = yes

#enable-auto-props = yes
enable-auto-props = yes

enable-auto-props = no

> Tools/Scripts/webkitpy/style/patchreader.py:60
> +        call_only_once = 1

Use True instead of 1.

> Tools/Scripts/webkitpy/style/patchreader.py:69
> +                    if(call_only_once):

Remove the ()s.

> Tools/Scripts/webkitpy/style/patchreader.py:73
> +                        if(det.display_name() == "git"):

Remove the ()s.

> Tools/Scripts/webkitpy/style/patchreader.py:74
> +                            call_only_once = 0

call_only_once = False

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