[webkit-reviews] review granted: [Bug 128422] Web Inspector: update check-webkit-style to flag single quotes in WebInspectorUI projects : [Attachment 224055] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Diego Pino
<dpino at igalia.com>'s request for review:
Bug 128422: Web Inspector: update check-webkit-style to flag single quotes in
WebInspectorUI projects
https://bugs.webkit.org/show_bug.cgi?id=128422

Attachment 224055: Patch
https://bugs.webkit.org/attachment.cgi?id=224055&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=224055&action=review


> Tools/Scripts/webkitpy/style/checkers/js.py:64
> +	       """FIXME: Despite stripping comments, there are two single-line
comments
> +	       not stripped in CodeMirrorAdditions.js. """
> +	       if (line.strip().find("//") == 0):
> +		   continue

Oh, what comments are these? The first part of the regex very clearly handles
'//.*?$' so its surprising to me that some might have been missed.

> Tools/Scripts/webkitpy/style/checkers/js.py:67
> +	       """Single-quotes within strings and regular expressions are
valid."""
> +	       if (self._within_string(line, "'") or self._within_regex(line,
"'")):
> +		   continue

Nit: You can just use python "# foo" comment syntax here. Unless there is an
advantage to the """....""" syntax that I don't know about. These just look
like generic inline comments.

> Tools/Scripts/webkitpy/style/checkers/js.py:71
> +	       if (pos != -1):
> +		   line_number, original_line =
self._find_original_line(content, line)
> +		   self._handle_style_error(line_number, "js/syntax", 5, "Line
contains single-quote character.")

This is pretty basic, which may be okay. It checks for the first single quote
and verifies that it is not within the first double quoted string on the list.
Likewise for regex. I don't think it can give false positives, but issues that
I think would go undetected include:

    foo("a'b", 'c');
    foo("a/'b", '/c');
    foo(/'/, 'c');

You might be able to detect these by (1) checking all the single quotes on a
line and (2) if you do a find and rfind around the position of the character
itself:
http://docs.python.org/2/library/string.html#string.find

Alternatively, now that comments are removed you could just search for single
quoted strings. The regular expression in _remove_comments looks like it would
already be sufficient if you ignore double quoted strings.

However, that being said our code tends to be rather straightforward, so this
looks good to me.