Skip to content

GitLab

T
tangled
Commit fb25a43f authored by Dan Beam's avatar Dan Beam Committed by Commit Bot
Browse files
Options

web_dev_style: add a presubmit for .bind(this) as it's discouraged by 

both Chromium and Google's JS style guides.

R=rbpotter@chromium.org
BUG=997405

Change-Id: I07ffb05cc2992e36f2d7674cf7660491f8f2e222
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1768950
Commit-Queue: Dan Beam <dbeam@chromium.org>
Auto-Submit: Dan Beam <dbeam@chromium.org>
Reviewed-by: default avatarRebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690211}
parent 6fe44208
Hide whitespace changes
Inline Side-by-side
Showing with 92 additions and 64 deletions
tools/web_dev_style/js_checker.py
View file @ fb25a43f
......@@ -20,31 +20,35 @@ class JSChecker(object):
return regex_check.RegexCheck(
self.input_api.re, line_number, line, regex, message)
def BindThisCheck(self, i, line):
return self.RegexCheck(i, line, r"(\.bind\(this)[^)]*\)",
"Prefer arrow (=>) functions over bind(this)");
def ChromeSendCheck(self, i, line):
"""Checks for a particular misuse of 'chrome.send'."""
"""Checks for a particular misuse of "chrome.send"."""
return self.RegexCheck(i, line, r"chrome\.send\('[^']+'\s*(, \[\])\)",
'Passing an empty array to chrome.send is unnecessary')
"Passing an empty array to chrome.send is unnecessary")
def CommentIfAndIncludeCheck(self, line_number, line):
return self.RegexCheck(line_number, line, r'(?<!\/\/ )(<if|<include) ',
'<if> or <include> should be in a single line comment with a space ' +
'after the slashes. Examples:\n' +
return self.RegexCheck(line_number, line, r"(?<!\/\/ )(<if|<include) ",
"<if> or <include> should be in a single line comment with a space " +
"after the slashes. Examples:\n" +
' // <include src="...">\n' +
' // <if expr="chromeos">\n' +
' // </if>\n')
" // </if>\n")
def EndJsDocCommentCheck(self, i, line):
msg = 'End JSDoc comments with */ instead of **/'
msg = "End JSDoc comments with */ instead of **/"
def _check(regex):
return self.RegexCheck(i, line, regex, msg)
return _check(r'^\s*(\*\*/)\s*$') or _check(r'/\*\* @[a-zA-Z]+.* (\*\*/)')
return _check(r"^\s*(\*\*/)\s*$") or _check(r"/\*\* @[a-zA-Z]+.* (\*\*/)")
def ExtraDotInGenericCheck(self, i, line):
return self.RegexCheck(i, line, r"((?:Array|Object|Promise)\.<)",
"Don't use a dot after generics (Object.<T> should be Object<T>).")
def InheritDocCheck(self, i, line):
"""Checks for use of '@inheritDoc' instead of '@override'."""
"""Checks for use of "@inheritDoc" instead of "@override"."""
return self.RegexCheck(i, line, r"\* (@inheritDoc)",
"@inheritDoc is deprecated, use @override instead")
......@@ -53,7 +57,7 @@ class JSChecker(object):
return self.RegexCheck(i, line, r"(?<!this)(\.\$)[\[\.](?![a-zA-Z]+\()",
"Please only use this.$.localId, not element.$.localId")
def RunEsLintChecks(self, affected_js_files, format='stylish'):
def RunEsLintChecks(self, affected_js_files, format="stylish"):
"""Runs lint checks using ESLint. The ESLint rules being applied are defined
in the .eslintrc.js configuration file.
"""
......@@ -64,7 +68,7 @@ class JSChecker(object):
for f in affected_js_files:
affected_js_files_paths.append(f.AbsoluteLocalPath())
args = ['--color', '--format', format, '--ignore-pattern \'!.eslintrc.js\'']
args = ["--color", "--format", format, "--ignore-pattern '!.eslintrc.js'"]
args += affected_js_files_paths
import eslint
......@@ -79,10 +83,10 @@ class JSChecker(object):
"Please use variable namesLikeThis <https://goo.gl/eQiXVW>")
def _GetErrorHighlight(self, start, length):
"""Takes a start position and a length, and produces a row of '^'s to
"""Takes a start position and a length, and produces a row of "^"s to
highlight the corresponding part of a string.
"""
return start * ' ' + length * '^'
return start * " " + length * "^"
def RunChecks(self):
"""Check for violations of the Chromium JavaScript style guide. See
......@@ -92,7 +96,7 @@ class JSChecker(object):
affected_files = self.input_api.AffectedFiles(file_filter=self.file_filter,
include_deletes=False)
affected_js_files = filter(lambda f: f.LocalPath().endswith('.js'),
affected_js_files = filter(lambda f: f.LocalPath().endswith(".js"),
affected_files)
if affected_js_files:
......@@ -114,13 +118,13 @@ class JSChecker(object):
if error_lines:
error_lines = [
'Found JavaScript style violations in %s:' %
"Found JavaScript style violations in %s:" %
f.LocalPath()] + error_lines
results.append(self.output_api.PresubmitError('\n'.join(error_lines)))
results.append(self.output_api.PresubmitError("\n".join(error_lines)))
if results:
results.append(self.output_api.PresubmitNotifyResult(
'See the JavaScript style guide at '
'https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#JavaScript'))
"See the JavaScript style guide at "
"https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md#JavaScript"))
return results
tools/web_dev_style/js_checker_test.py
View file @ fb25a43f
......@@ -11,12 +11,12 @@ import test_util
import unittest
_HERE = os_path.dirname(os_path.abspath(__file__))
sys_path.append(os_path.join(_HERE, '..', '..'))
sys_path.append(os_path.join(_HERE, "..", ".."))
from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi
_DECLARATION_METHODS = 'const', 'let', 'var'
_DECLARATION_METHODS = "const", "let", "var"
class JsCheckerTest(unittest.TestCase):
......@@ -25,17 +25,46 @@ class JsCheckerTest(unittest.TestCase):
self.checker = js_checker.JSChecker(MockInputApi(), MockOutputApi())
def ShouldFailBindThisCheck(self, line):
error = self.checker.BindThisCheck(1, line)
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
highlight = test_util.GetHighlight(line, error).strip()
self.assertTrue(highlight.startswith(".bind(this"))
def ShouldPassBindThisCheck(self, line):
self.assertEqual("", self.checker.BindThisCheck(1, line),
"Should not be flagged as style error: " + line)
def testBindThisFails(self):
lines = [
'let bound = this.method_.bind(this);',
"cr.doc.addEventListener('click', this.onClick_.bind(this));",
'this.api_.onEvent = this.onClick_.bind(this);',
'this.api_.getThinger(this.gotThinger_.bind(this));',
'this.api_.getThinger(this.gotThinger_.bind(this, param1, param2));',
]
for line in lines:
self.ShouldFailBindThisCheck(line)
def testBindThisPasses(self):
lines = [
'// And in the darkness bind them.',
'this.methodName_.bind(scope, param)',
]
for line in lines:
self.ShouldPassBindThisCheck(line)
def ShouldFailCommentCheck(self, line):
"""Checks that uncommented '<if>' and '<include>' are a style error."""
"""Checks that uncommented "<if>" and "<include>" are a style error."""
error = self.checker.CommentIfAndIncludeCheck(1, line)
self.assertNotEqual('', error, 'Should be flagged as style error: ' + line)
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
highlight = test_util.GetHighlight(line, error).strip()
self.assertTrue(highlight.startswith(('<if', '<include')))
self.assertTrue(highlight.startswith(("<if", "<include")))
def ShouldPassCommentCheck(self, line):
"""Checks that commented '<if>' and '<include>' are allowed."""
self.assertEqual('', self.checker.CommentIfAndIncludeCheck(1, line),
'Should not be flagged as style error: ' + line)
"""Checks that commented "<if>" and "<include>" are allowed."""
self.assertEqual("", self.checker.CommentIfAndIncludeCheck(1, line),
"Should not be flagged as style error: " + line)
def testCommentFails(self):
lines = [
......@@ -64,18 +93,17 @@ class JsCheckerTest(unittest.TestCase):
self.ShouldPassCommentCheck(line)
def ShouldFailChromeSendCheck(self, line):
"""Checks that the 'chrome.send' checker flags |line| as a style error."""
"""Checks that the "chrome.send" checker flags |line| as a style error."""
error = self.checker.ChromeSendCheck(1, line)
self.assertNotEqual('', error,
'Should be flagged as style error: ' + line)
self.assertEqual(test_util.GetHighlight(line, error), ', []')
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
self.assertEqual(test_util.GetHighlight(line, error), ", []")
def ShouldPassChromeSendCheck(self, line):
"""Checks that the 'chrome.send' checker doesn't flag |line| as a style
"""Checks that the "chrome.send" checker doesn"t flag |line| as a style
error.
"""
self.assertEqual('', self.checker.ChromeSendCheck(1, line),
'Should not be flagged as style error: ' + line)
self.assertEqual("", self.checker.ChromeSendCheck(1, line),
"Should not be flagged as style error: " + line)
def testChromeSendFails(self):
lines = [
......@@ -87,10 +115,10 @@ class JsCheckerTest(unittest.TestCase):
def testChromeSendPasses(self):
lines = [
"chrome.send('message', constructArgs('foo', []));",
" chrome.send('message', constructArgs('foo', []));",
"chrome.send('message', constructArgs([]));",
" chrome.send('message', constructArgs([]));",
'chrome.send("message", constructArgs("foo", []));',
' chrome.send("message", constructArgs("foo", []));',
'chrome.send("message", constructArgs([]));',
' chrome.send("message", constructArgs([]));',
]
for line in lines:
self.ShouldPassChromeSendCheck(line)
......@@ -98,14 +126,13 @@ class JsCheckerTest(unittest.TestCase):
def ShouldFailEndJsDocCommentCheck(self, line):
"""Checks that the **/ checker flags |line| as a style error."""
error = self.checker.EndJsDocCommentCheck(1, line)
self.assertNotEqual('', error,
'Should be flagged as style error: ' + line)
self.assertEqual(test_util.GetHighlight(line, error), '**/')
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
self.assertEqual(test_util.GetHighlight(line, error), "**/")
def ShouldPassEndJsDocCommentCheck(self, line):
"""Checks that the **/ checker doesn't flag |line| as a style error."""
self.assertEqual('', self.checker.EndJsDocCommentCheck(1, line),
'Should not be flagged as style error: ' + line)
"""Checks that the **/ checker doesn"t flag |line| as a style error."""
self.assertEqual("", self.checker.EndJsDocCommentCheck(1, line),
"Should not be flagged as style error: " + line)
def testEndJsDocCommentFails(self):
lines = [
......@@ -131,7 +158,7 @@ class JsCheckerTest(unittest.TestCase):
def ShouldFailExtraDotInGenericCheck(self, line):
"""Checks that Array.< or Object.< is flagged as a style nit."""
error = self.checker.ExtraDotInGenericCheck(1, line)
self.assertNotEqual('', error)
self.assertNotEqual("", error)
self.assertTrue(test_util.GetHighlight(line, error).endswith(".<"))
def testExtraDotInGenericFails(self):
......@@ -144,18 +171,17 @@ class JsCheckerTest(unittest.TestCase):
self.ShouldFailExtraDotInGenericCheck(line)
def ShouldFailInheritDocCheck(self, line):
"""Checks that the '@inheritDoc' checker flags |line| as a style error."""
"""Checks that the "@inheritDoc" checker flags |line| as a style error."""
error = self.checker.InheritDocCheck(1, line)
self.assertNotEqual('', error,
msg='Should be flagged as style error: ' + line)
self.assertEqual(test_util.GetHighlight(line, error), '@inheritDoc')
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
self.assertEqual(test_util.GetHighlight(line, error), "@inheritDoc")
def ShouldPassInheritDocCheck(self, line):
"""Checks that the '@inheritDoc' checker doesn't flag |line| as a style
"""Checks that the "@inheritDoc" checker doesn"t flag |line| as a style
error.
"""
self.assertEqual('', self.checker.InheritDocCheck(1, line),
msg='Should not be flagged as style error: ' + line)
self.assertEqual("", self.checker.InheritDocCheck(1, line),
msg="Should not be flagged as style error: " + line)
def testInheritDocFails(self):
lines = [
......@@ -178,22 +204,21 @@ class JsCheckerTest(unittest.TestCase):
def ShouldFailPolymerLocalIdCheck(self, line):
"""Checks that element.$.localId check marks |line| as a style error."""
error = self.checker.PolymerLocalIdCheck(1, line)
self.assertNotEqual('', error,
msg='Should be flagged as a style error: ' + line)
self.assertTrue('.$' in test_util.GetHighlight(line, error))
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
self.assertTrue(".$" in test_util.GetHighlight(line, error))
def ShouldPassPolymerLocalIdCheck(self, line):
"""Checks that element.$.localId check doesn't mark |line| as a style
error."""
self.assertEqual('', self.checker.PolymerLocalIdCheck(1, line),
msg='Should not be flagged as a style error: ' + line)
self.assertEqual("", self.checker.PolymerLocalIdCheck(1, line),
msg="Should not be flagged as a style error: " + line)
def testPolymerLocalIdFails(self):
lines = [
"cat.$.dog",
"thing1.$.thing2",
"element.$.localId",
"element.$['fancy-hyphenated-id']",
'element.$["fancy-hyphenated-id"]',
]
for line in lines:
self.ShouldFailPolymerLocalIdCheck(line)
......@@ -202,7 +227,7 @@ class JsCheckerTest(unittest.TestCase):
lines = [
"this.$.id",
"this.$.localId",
"this.$['fancy-id']",
'this.$["fancy-id"]',
"this.page.$.flushForTesting()",
]
for line in lines:
......@@ -211,15 +236,14 @@ class JsCheckerTest(unittest.TestCase):
def ShouldFailVariableNameCheck(self, line):
"""Checks that var unix_hacker, $dollar are style errors."""
error = self.checker.VariableNameCheck(1, line)
self.assertNotEqual('', error,
msg='Should be flagged as style error: ' + line)
self.assertNotEqual("", error, "Should be flagged as style error: " + line)
highlight = test_util.GetHighlight(line, error)
self.assertFalse(any(dm in highlight for dm in _DECLARATION_METHODS))
def ShouldPassVariableNameCheck(self, line):
"""Checks that variableNamesLikeThis aren't style errors."""
self.assertEqual('', self.checker.VariableNameCheck(1, line),
msg='Should not be flagged as style error: ' + line)
"""Checks that variableNamesLikeThis aren"t style errors."""
self.assertEqual("", self.checker.VariableNameCheck(1, line),
msg="Should not be flagged as style error: " + line)
def testVariableNameFails(self):
lines = [
......@@ -252,5 +276,5 @@ class JsCheckerTest(unittest.TestCase):
self.ShouldPassVariableNameCheck(line % declaration_method)
if __name__ == '__main__':
if __name__ == "__main__":
unittest.main()
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment