[webkit-reviews] review denied: [Bug 75824] check-webkit-style should warn about missing svn:mime-type for png files : [Attachment 132787] 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 132787: proposed fix
https://bugs.webkit.org/attachment.cgi?id=132787&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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