Bug 182405 – prepare-ChangeLog gets confused about Python docstrings that contain the word "class" or "def"

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

When I ran prepare-ChangeLog to generate the ChangeLog for the changes in attachment #332849 [details] (bug #182360) the ChangeLog had entries of the form 

    * Scripts/webkitpy/layout_tests/servers/websocket_server.py:
    (to):
    ...

    * Scripts/webkitpy/port/base.py:
    (Port.to.is_http_server_running):
    ...

But there is neither a class called "to" in file Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py nor a class Port.to in file Tools/Scripts/webkitpy/port/base.py.

The top of the file Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py contains the following docstring, <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py?rev=227942#L30>:
    """A class to help start/stop the PyWebSocket server used by layout tests."""

And Tools/Scripts/webkitpy/port/base.py has the following docstring immediate that is indented inside the scope of class Port and before the definition of Port.is_http_server_running(), <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/base.py?rev=227942#L69>:
    """Abstract class for Port-specific hooks for the layout_test package."""

Notice that both of these docstrings contain the word "class".

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 332909 [details]
Patch and tests

[EWS Watchlist]

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.

[JF Bastien]

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 :)

[Daniel Bates]

(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.

[Daniel Bates]

Created attachment 333111 [details]
Patch and tests

[Daniel Bates]

Created attachment 333112 [details]
Patch and tests

[EWS Watchlist]

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.

[David Kilzer (:ddkilzer)]

Comment on attachment 333112 [details]
Patch and tests

r=me

[Daniel Bates]

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;

[Daniel Bates]

Committed r228131: <https://trac.webkit.org/changeset/228131>

[Radar WebKit Bug Importer]

<rdar://problem/37251100>