[webkit-reviews] review granted: [Bug 75202] Rewrite the CSS parser of prepare-ChangeLog with unittests : [Attachment 121095] rebased patch for review

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 75202: Rewrite the CSS parser of prepare-ChangeLog with unittests
https://bugs.webkit.org/show_bug.cgi?id=75202

Attachment 121095: rebased patch for review
https://bugs.webkit.org/attachment.cgi?id=121095&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121095&action=review


It's great to be able to test this code!  r=me

I would really like to see test cases for the warn conditions, and please
consider listing the method instead of just the language in %testFiles, but
that shouldn't block landing this patch.

> Tools/Scripts/prepare-ChangeLog:1385
> +	   foreach my $token (split m-(\{|\}|/\*|\*/)-, $_) {

Nit: I've typically seen m## used instead of m-- when m// is undesirable, but
this is okay.

> Tools/Scripts/prepare-ChangeLog:1388
> +		       warn "mismatched brace found in $fileName\n" if
$inBrace;

Would be nice to have a test for this case.

> Tools/Scripts/prepare-ChangeLog:1393
> +		       warn "mismatched brace found in $fileName\n" if
!$inBrace;

Would be nice to have a test for this case.

> Tools/Scripts/prepare-ChangeLog:1402
> +		   warn "mismatched comment found in $fileName\n" if
!$inComment;

Would be nice to have a test for this case.

> Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/parser_unittests.pl:59
> +    my $parser = $test->{language} eq "css" ?
> +	  eval "\\&PrepareChangeLog::get_selector_line_ranges_for_css" :
> +	  eval
"\\&PrepareChangeLog::get_function_line_ranges_for_$test->{language}";

At this point, I think it would be easier to store the method name instead of
just the language in %testFiles:

my %testFiles = (
    "perl_unittests.pl" => "get_function_line_ranges_for_perl",
    "python_unittests.py" => "get_function_line_ranges_for_python",
    "java_unittests.java" => "get_function_line_ranges_for_java",
    "cpp_unittests.cpp" => "get_function_line_ranges_for_cpp",
    "css_unittests.css" => "get_selector_line_ranges_for_css",
);

Then this becomes simpler to read (change 'language' to 'method' in @testSet):

    my $parser = eval "\\&PrepareChangeLog::$test->{method}";

I'm not going to withhold a review for this, but I think you should consider
it.