[webkit-reviews] review granted: [Bug 34260] check-webkit-style: is not reporting the carriage-return error for patches : [Attachment 47867] Proposed patch

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has granted Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 34260: check-webkit-style: is not reporting the carriage-return error for
patches
https://bugs.webkit.org/show_bug.cgi?id=34260

Attachment 47867: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=47867&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
>      def __init__(self, output_format="emacs", verbosity=1, filter=None,
> -		    git_commit=None, extra_flag_values=None):
> -	   if filter is None:
> -	       filter = CategoryFilter()
> +		    max_reports_per_category=None,
> +		    git_commit=None,
> +		    extra_flag_values=None):

Though this code is OK as is, I'd break line for each arguments to improve the
consistency a bit:

      def __init__(self,
		   output_format="emacs",
		   verbosity=1,
		   filter=None,
		   max_reports_per_category=None,
		   git_commit=None,
		   extra_flag_values=None):

> +	   # FIXME: Shouldn't we always do this check?

Agreed. I guess we need to use svn property "eol-style" or a whitelist instead
of this check.

> +    def test_max_reports_per_category(self):
> +	   """Check that MAX_REPORTS_PER_CATEGORY is valid."""
> +	   categories = style.style_categories()
> +	   for category in style.MAX_REPORTS_PER_CATEGORY.keys():

I slightly prefer iterkeys, but it's OK as is.

> +    def test_ends_with_carriage_newline(self):
> +	   # Check_no_carriage_return only checks the final character.
> +	   self.assert_carriage_return("no carriage return\r\n",
is_error=False)

This test case looks weird as we want to warn for this case. I think we'll
never pass strings which contain "\n" as a parameter of
check_no_carriage_return. How about using "carriage return\r in a string" or
something like this?