Summary: | prepare-ChangeLog gets confused about Python docstrings that contain the word "class" or "def" | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||||||
Component: | Tools / Tests | Assignee: | Daniel Bates <dbates> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aakash_jain, ddkilzer, ews-watchlist, jfbastien, lforschler, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Daniel Bates
2018-02-01 13:26:10 PST
Created attachment 332904 [details] Test Attached is the code changes portion of the patch from attachment #332849 [details] (bug #182360). Steps to reproduce: 1. Download the patch to ~/Desktop/test.diff. 2. From the top-level WebKit checkout, run `Tools/Scripts/svn-apply ~/Desktop/test.diff`. 3. From the top-level WebKit checkout, run `Tools/Scripts/prepare-ChangeLog --no-write`. Then the function list portion of the ChangeLog will be: * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py: (LayoutTestRunner.__init__): (LayoutTestRunner.start_servers): (LayoutTestRunner.stop_servers): * Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py: (LayoutTestRunnerTests.test_servers_started.is_websocket_server_running): (LayoutTestRunnerTests.test_servers_started): (LayoutTestRunnerTests.test_servers_started.is_websocket_servers_running): Deleted. * Scripts/webkitpy/layout_tests/servers/websocket_server.py: (to): (PyWebSocket.__init__): (PyWebSocket._stop_running_server): (is_web_socket_server_running): (PyWebSocket.is_running): Deleted. * Scripts/webkitpy/port/base.py: (Port.to.is_http_server_running): (Port.to.is_websocket_server_running): (Port.to.is_wpt_server_running): (Port.to.is_websocket_servers_running): Deleted. But it should be: * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py: (LayoutTestRunner.__init__): (LayoutTestRunner.start_servers): (LayoutTestRunner.stop_servers): * Scripts/webkitpy/layout_tests/controllers/layout_test_runner_unittest.py: (LayoutTestRunnerTests.test_servers_started.is_websocket_server_running): (LayoutTestRunnerTests.test_servers_started): (LayoutTestRunnerTests.test_servers_started.is_websocket_servers_running): Deleted. * Scripts/webkitpy/layout_tests/servers/websocket_server.py: (PyWebSocket.__init__): (PyWebSocket._stop_running_server): (is_web_socket_server_running): (PyWebSocket.is_running): Deleted. * Scripts/webkitpy/port/base.py: (Port.is_http_server_running): (Port.is_websocket_server_running): (Port.is_wpt_server_running): (Port.is_websocket_servers_running): Deleted. Created attachment 332909 [details]
Patch and tests
Attachment 332909 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/python_unittests.py:112: expected 2 blank lines, found 0 [pep8/E302] [5]
ERROR: Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/python_unittests.py:119: expected 2 blank lines, found 0 [pep8/E302] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 332909 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=332909&action=review r=me > Tools/Scripts/prepare-ChangeLog:1798 > + next unless /^(\s*)([^#].*)$/; # Skip non-indented lines that begin with a comment. I don't think this is accurate: the regex matches any line that contains code (starts with spaces, contains a non-hash character, and then anything). It'll stop if there's an empty line (potentially indented) as well as a comment-only line (potentially indented). > Tools/Scripts/prepare-ChangeLog:1827 > + next if /^\s*[#'"]/; # Skip indented and non-indented lines that begin with a comment or string literal (includes docstrings). That won't capture multi-line docstrings though. I guess it's a net improvement over what was there before, so only fix if you think it matters :) (In reply to JF Bastien from comment #4) > Comment on attachment 332909 [details] > Patch and tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332909&action=review > > r=me > > > Tools/Scripts/prepare-ChangeLog:1798 > > + next unless /^(\s*)([^#].*)$/; # Skip non-indented lines that begin with a comment. > > I don't think this is accurate: the regex matches any line that contains > code (starts with spaces, contains a non-hash character, and then anything). > It'll stop if there's an empty line (potentially indented) as well as a > comment-only line (potentially indented). > Notice that when the regex matches then we process the line. That is, we do not skip it. An indented comment-only line will match /^(\s*)([^#].*)$/ because [^#] will match a whitespace character and \s* matches arbitrary length whitespace (Perl will backtrack through the characters captured by \s* to match [^#]); => we will process this line; => we will not skip this line. For example, " #\n" will match with $1 = "" and $2 = " #". So, we will process this line. In contrast, a non-indented comment-only line will not match /^(\s*)([^#].*)$/ because the first character in the line will not match [^#] (since it is # by definition); => we will not process the line; => will skip the line. So, "next unless /^(\s*)([^#].*)$/" means skip a non-indented comment-only line. Maybe there is a better way to write this logic to make it clearer? > > Tools/Scripts/prepare-ChangeLog:1827 > > + next if /^\s*[#'"]/; # Skip indented and non-indented lines that begin with a comment or string literal (includes docstrings). > > That won't capture multi-line docstrings though. I guess it's a net > improvement over what was there before, so only fix if you think it matters > :) You're right! Will fix. Created attachment 333111 [details]
Patch and tests
Created attachment 333112 [details]
Patch and tests
Attachment 333112 [details] did not pass style-queue:
ERROR: Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/python_unittests.py:112: expected 2 blank lines, found 0 [pep8/E302] [5]
ERROR: Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/python_unittests.py:119: expected 2 blank lines, found 0 [pep8/E302] [5]
ERROR: Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/resources/python_unittests.py:136: expected 2 blank lines, found 0 [pep8/E302] [5]
Total errors found: 3 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333112 [details]
Patch and tests
r=me
Comment on attachment 333112 [details] Patch and tests View in context: https://bugs.webkit.org/attachment.cgi?id=333112&action=review > Tools/Scripts/prepare-ChangeLog:1837 > + # Skip multi-line string literals and docstrings. > + next if /^\s*$multilineStringLiteralSentinelRegEx.*$multilineStringLiteralSentinelRegEx\s*$/; > + if (!$inComment && /^\s*$multilineStringLiteralSentinelRegEx/) { > + $inComment = 1; > + } elsif ($inComment && /$multilineStringLiteralSentinelRegEx\s*$/) { > + $inComment = 0; > + } > + next if $inComment; Before landing, will extract starting and ending multi-line patterns into local variables defined after line 1786: my $multilineStringLiteralStartRegEx = qr#^\s*$multilineStringLiteralSentinelRegEx#; my $multilineStringLiteralEndRegEx = qr#$multilineStringLiteralSentinelRegEx\s*$#; And then write these lines as: # Skip multi-line string literals and docstrings. next if /$multilineStringLiteralStartRegEx.*$multilineStringLiteralEndRegEx/; if (!$inComment && /$multilineStringLiteralStartRegEx/) { $inComment = 1; } elsif ($inComment && /$multilineStringLiteralEndRegEx/) { $inComment = 0; } next if $inComment; Committed r228131: <https://trac.webkit.org/changeset/228131> |