[webkit-reviews] review granted: [Bug 171424] check-webkit-style should keep JavaScript test functions in sync : [Attachment 308661] Patch v1 for Human Review

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 171424: check-webkit-style should keep JavaScript test functions in sync
https://bugs.webkit.org/show_bug.cgi?id=171424

Attachment 308661: Patch v1 for Human Review

https://bugs.webkit.org/attachment.cgi?id=308661&action=review




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 308661
  --> https://bugs.webkit.org/attachment.cgi?id=308661
Patch v1 for Human Review

View in context: https://bugs.webkit.org/attachment.cgi?id=308661&action=review

r=me

> Tools/Scripts/webkitpy/style/checkers/jstest.py:54
> +    function_name_regex =
re.compile('^(?P<name>[_a-zA-Z][_a-zA-Z0-9]*)\s*\(.*', flags=re.MULTILINE)

Nit: The `.*` at the end of this regular expression can be dropped. It can
match nothing, or anything, and is unused. So I think just ending in a \( is
good enough.

This approach is a bit peculiar:

  (1) Split of `function\s+`
  (2) remove trailing blank lines and comments that are the dead space between
when this function ended and the next function

But I suppose its fine given we don't want to be dealing with balancing braces
and the format of these tests files is unlikely to change.

> Tools/Scripts/webkitpy/style/checkers/jstest.py:63
> +def strip_blank_lines_and_comments(function):

These only strip them from the end, so maybe rename this to strip_trailing_...

> Tools/Scripts/webkitpy/style/checkers/jstest.py:124
> +						    "Changes to function {0}()
should be kept in sync with {1}.".format(
> +							function_name, path))

Nit: I don't find this line break particularly helpful.