[Webkit-unassigned] [Bug 128422] Web Inspector: update check-webkit-style to flag single quotes in WebInspectorUI projects

[bugzilla-daemon at webkit.org]

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


Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #224055|review?                     |review+
               Flag|                            |




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org>  2014-02-17 17:06:03 PST ---
(From update of attachment 224055)
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.

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