[Webkit-unassigned] [Bug 75825] webkit-patch land should automatically add svn:mime-type for .png files

[bugzilla-daemon at webkit.org]

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


Peter Gal <galpeter at inf.u-szeged.hu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |galpeter at inf.u-szeged.hu




--- Comment #29 from Peter Gal <galpeter at inf.u-szeged.hu>  2012-06-19 02:04:25 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=147802&action=review

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:32
> +def check(self, host, fs):

I think the 'self' here is a little misleading, and why do we need it at all?

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:52
> +def _config_file_path(self, host, fs):

Why do you need the 'self'? It seems it is unused.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:53
> +    config_file = ""

I don't see this variable used in this method.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:57
> +        config_file_path = fs.join(os.environ['APPDATA'], "Subversion\config")
> +    else:
> +        config_file_path = fs.join(fs.expanduser("~"), ".subversion/config")

fs.join(..) can take more than 2 arguments, the config should be the 3.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:62
> +    return "Have to enable auto props in the subversion config file (" + config_file_path + " \"enable-auto-props = yes\"). "

Instead of escaping the " use ' at the start and end. Oh.. and use the %s formatter, that is more readable.

> Tools/Scripts/webkitpy/common/checksvnconfigfile.py:66
> +    return "Have to set the svn:mime-type in the subversion config file (" + config_file_path + " \"*.png = svn:mime-type=image/png\")."

ditto

> Tools/Scripts/webkitpy/style/checkers/png.py:59
> +            config_file_path = checksvnconfigfile._config_file_path(self, self._host, self._fs)

Calling a method prefixed with underscore isn't really nice. Ditch the leading underscore.

> Tools/Scripts/webkitpy/style/checkers/png.py:62
> +                self._handle_style_error(0, 'image/png', 5, "There is no SVN config file. (" + config_file_path + ")")

use %s formatter

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:42
> +        if len(png_files) > 0:

No need for length check, just: "if png_files:"

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:46
> +                (file_read, autoprop, png) = checksvnconfigfile.check(self, self._host, self._fs)

use the same naming like in the PNGChecker.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng.py:68
> +                    if filename.endswith('.png') and self._detector.exists(filename) and self._detector.propget('svn:mime-type', filename) != 'image/png':

The file extension is already checked in the _check_pngs method. Why do you check it again?

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:46
> +        options = MockOptions()
> +        options.git_commit = 'MOCK git commit'

Pass the git_commit as a kwarg to the MockOptions.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:54
> +            "changed_files": ["test.png"],

Add a non-png file to the list.

> Tools/Scripts/webkitpy/tool/steps/addsvnmimetypeforpng_unittest.py:59
> +        except SystemExit, e:
> +            self.assertEquals(type(e), type(SystemExit()))

Why do we check if the type is a SystemExit exception? The except will only catch SystemExit ones.

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