squah-confluent commented on code in PR #23691:
URL: https://github.com/apache/kafka/pull/23691#discussion_r4305936011
##########
.github/workflows/pr-linter.yml:
##########
@@ -52,6 +52,8 @@ jobs:
description: 'Could not load PR number'
context: 'PR Linter'
state: 'error'
+ - name: Setup Python
Review Comment:
This step is no longer needed now that there's no new dependency, and it
runs `pip install` in the job that holds the PR-editing token. Could it be
removed?
##########
.github/scripts/pr-format.py:
##########
@@ -18,14 +18,15 @@
import json
import logging
import os
+import re
import subprocess
import shlex
import sys
import tempfile
import textwrap
from typing import Dict, Optional, TextIO
-logger = logging.getLogger()
+logger = logging.getLogger("pr-format")
Review Comment:
This rename was only needed to quiet the parser's debug output. Could it be
reverted?
##########
.github/scripts/test_pr_format.py:
##########
@@ -0,0 +1,109 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import importlib.util
+import subprocess
+import unittest
+from pathlib import Path
+
+
+SCRIPT = Path(__file__).with_name("pr-format.py")
+SPEC = importlib.util.spec_from_file_location("pr_format", SCRIPT)
+pr_format = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(pr_format)
+
+
+class FormatBodyTest(unittest.TestCase):
+ def test_wraps_plain_prose_at_72_columns(self):
+ body = (
+ "This is a plain paragraph with enough words to wrap at the
expected "
+ "column while keeping every word and the paragraph meaning
intact.\n"
+ )
+
+ formatted = pr_format.format_body(body)
+
+ self.assertEqual(
+ "This is a plain paragraph with enough words to wrap at the
expected\n"
+ "column while keeping every word and the paragraph meaning
intact.\n",
+ formatted,
+ )
+ self.assertTrue(all(len(line) <= 72 for line in
formatted.splitlines()))
+
+ def test_preserves_indented_list_reproduction(self):
Review Comment:
This only covers ` - `. Could we parametrize over the other list markers,
with and without indentation and with and without a space after the marker?
Lists without a space after the marker aren't valid Markdown list syntax,
but the lines are clearly meant to stay separate in a commit message. At the
moment, unindented `-first item` and `+first item` are joined into the previous
line:
```
Example list: -first item -second item
```
```python
def test_preserves_list_markers(self):
prefixes = [
"".join(p) for p in itertools.product(("", " "), ("-", "+",
"*"), ("", " "))
]
for prefix in prefixes + ["1. ", "1) "]:
with self.subTest(prefix=prefix):
body = f"Example list:\n{prefix}first item\n{prefix}second
item\n"
self.assertEqual(body, pr_format.format_body(body))
```
##########
.github/scripts/test_pr_format.py:
##########
@@ -0,0 +1,109 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import importlib.util
+import subprocess
+import unittest
+from pathlib import Path
+
+
+SCRIPT = Path(__file__).with_name("pr-format.py")
+SPEC = importlib.util.spec_from_file_location("pr_format", SCRIPT)
+pr_format = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(pr_format)
+
+
+class FormatBodyTest(unittest.TestCase):
+ def test_wraps_plain_prose_at_72_columns(self):
+ body = (
+ "This is a plain paragraph with enough words to wrap at the
expected "
+ "column while keeping every word and the paragraph meaning
intact.\n"
+ )
+
+ formatted = pr_format.format_body(body)
+
+ self.assertEqual(
+ "This is a plain paragraph with enough words to wrap at the
expected\n"
+ "column while keeping every word and the paragraph meaning
intact.\n",
+ formatted,
+ )
+ self.assertTrue(all(len(line) <= 72 for line in
formatted.splitlines()))
+
+ def test_preserves_indented_list_reproduction(self):
+ body = "Example list:\n - first item\n - second item\n"
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_fenced_code_and_long_indented_lines(self):
+ body = (
+ "```text\n"
+ "first code line\n"
+ "\n"
+ " " + ("long code content " * 8) + "\n"
+ "last code line\n"
+ "```\n"
+ )
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_inline_markdown_and_aligned_tables(self):
+ body = (
+ "This paragraph has `inline code` and [a
link](https://example.com).\n\n"
+ "| Name | Value |\n"
+ "|-------|-------|\n"
+ "| alpha | beta |\n"
+ )
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_reviewer_continuations_remain_parseable(self):
+ body = (
+ "Reviewers: Ada Lovelace <[email protected]>, Grace Hopper "
+ "<[email protected]>, Katherine Johnson <[email protected]>\n"
+ )
+
+ formatted = pr_format.format_body(body)
+
+ self.assertIn("\n ", formatted)
+ result = subprocess.run(
+ ["git", "interpret-trailers", "--trim-empty", "--parse"],
+ input=("KAFKA-21205 preserve Markdown\n\n" + formatted).encode(),
+ capture_output=True,
+ check=True,
+ )
+ self.assertEqual(
+ b"Reviewers: Ada Lovelace <[email protected]>, Grace Hopper
<[email protected]>, "
+ b"Katherine Johnson <[email protected]>\n",
+ result.stdout,
+ )
+
+ def test_formatting_is_idempotent(self):
Review Comment:
Idempotence is only checked here, on one input. The linter runs again every
time the PR description is edited, so every input should be stable. Could each
test end with an idempotence assert instead?
```python
self.assertEqual(formatted, pr_format.format_body(formatted))
```
That would also cover the new cases suggested on the other tests. If the JMH
case above isn't handled, it's worth knowing it's currently the one input that
isn't idempotent.
##########
.github/scripts/test_pr_format.py:
##########
@@ -0,0 +1,109 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import importlib.util
+import subprocess
+import unittest
+from pathlib import Path
+
+
+SCRIPT = Path(__file__).with_name("pr-format.py")
+SPEC = importlib.util.spec_from_file_location("pr_format", SCRIPT)
+pr_format = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(pr_format)
+
+
+class FormatBodyTest(unittest.TestCase):
+ def test_wraps_plain_prose_at_72_columns(self):
+ body = (
+ "This is a plain paragraph with enough words to wrap at the
expected "
+ "column while keeping every word and the paragraph meaning
intact.\n"
+ )
+
+ formatted = pr_format.format_body(body)
+
+ self.assertEqual(
+ "This is a plain paragraph with enough words to wrap at the
expected\n"
+ "column while keeping every word and the paragraph meaning
intact.\n",
+ formatted,
+ )
+ self.assertTrue(all(len(line) <= 72 for line in
formatted.splitlines()))
+
+ def test_preserves_indented_list_reproduction(self):
+ body = "Example list:\n - first item\n - second item\n"
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_fenced_code_and_long_indented_lines(self):
+ body = (
+ "```text\n"
+ "first code line\n"
+ "\n"
+ " " + ("long code content " * 8) + "\n"
+ "last code line\n"
+ "```\n"
+ )
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_inline_markdown_and_aligned_tables(self):
Review Comment:
The prose line here is 67 characters, so this test passes whether or not
paragraphs with inline Markdown get wrapped.
I'm also not sure these paragraphs should be left alone. Any paragraph
containing inline code, underscores or links is never wrapped. Trunk wraps this
paragraph, but after this PR it is left as one 127-character line:
```
This change makes `GroupCoordinatorService` validate the
`group.consumer.session.timeout.ms` config before the group is loaded.
```
In practice that means most Kafka PR descriptions won't be wrapped. Could
paragraphs with inline code, underscores and links be wrapped too, with a test
that wraps a long paragraph like the one above? Wrapping only breaks lines at
spaces, so a link's text may be split across lines, but the PR description on
GitHub still renders it as a single link.
Paragraphs made of deliberately separate lines are still reflowed, as they
are on trunk. This timeline, adapted from
[90a4c93](https://github.com/apache/kafka/commit/90a4c9397902044935ac566e2cf4b828bdb1d36d):
```
+04.151 consumer1 starts poll()
+04.877 consumer1 heartbeats
+05.155 consumer1 ends poll()
```
becomes, both before and after this PR,
```
+04.151 consumer1 starts poll() +04.877 consumer1 heartbeats +05.155
consumer1 ends poll()
```
(After this PR it only survives when a line happens to contain `[` or `]`,
as the original in that commit does.) Treating lines that start with `+` or `-`
like the list markers on `test_preserves_indented_list_reproduction` would
cover this too.
Unfenced aligned output is similar. This JMH result from
[9f15f3c](https://github.com/apache/kafka/commit/9f15f3c540eccfbff4dd4cc4bcb93e2e55750185):
```
Benchmark Mode Cnt
Score Error Units
ProducerMetadataAddBenchmark.addExistingTopicFullySynchronized thrpt 5
6217.287 ± 1279.054 ops/ms
```
becomes, both before and after this PR,
```
Benchmark Mode
Cnt Score Error Units
ProducerMetadataAddBenchmark.addExistingTopicFullySynchronized thrpt
5 6217.287 ± 1279.054 ops/ms
```
and a second run changes it again (`thrpt 5` / `6217.287 ...`). This one is
optional: it would be good to handle if it can be done without affecting
ordinary prose or adding much code, but it's fine to leave it otherwise, since
such output really belongs in a code fence. The obvious rule, leaving any
paragraph with a run of 3+ spaces unwrapped, isn't a good fit: it would also
stop wrapping ordinary prose and `Reviewers:` lines that happen to contain
stray spaces.
##########
.github/scripts/test_pr_format.py:
##########
@@ -0,0 +1,109 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import importlib.util
+import subprocess
+import unittest
+from pathlib import Path
+
+
+SCRIPT = Path(__file__).with_name("pr-format.py")
+SPEC = importlib.util.spec_from_file_location("pr_format", SCRIPT)
+pr_format = importlib.util.module_from_spec(SPEC)
+SPEC.loader.exec_module(pr_format)
+
+
+class FormatBodyTest(unittest.TestCase):
+ def test_wraps_plain_prose_at_72_columns(self):
+ body = (
+ "This is a plain paragraph with enough words to wrap at the
expected "
+ "column while keeping every word and the paragraph meaning
intact.\n"
+ )
+
+ formatted = pr_format.format_body(body)
+
+ self.assertEqual(
+ "This is a plain paragraph with enough words to wrap at the
expected\n"
+ "column while keeping every word and the paragraph meaning
intact.\n",
+ formatted,
+ )
+ self.assertTrue(all(len(line) <= 72 for line in
formatted.splitlines()))
+
+ def test_preserves_indented_list_reproduction(self):
+ body = "Example list:\n - first item\n - second item\n"
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_fenced_code_and_long_indented_lines(self):
+ body = (
+ "```text\n"
+ "first code line\n"
+ "\n"
+ " " + ("long code content " * 8) + "\n"
+ "last code line\n"
+ "```\n"
+ )
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_preserves_inline_markdown_and_aligned_tables(self):
+ body = (
+ "This paragraph has `inline code` and [a
link](https://example.com).\n\n"
+ "| Name | Value |\n"
+ "|-------|-------|\n"
+ "| alpha | beta |\n"
+ )
+
+ self.assertEqual(body, pr_format.format_body(body))
+
+ def test_reviewer_continuations_remain_parseable(self):
Review Comment:
Could we add cases where `Reviewers:` is next to another trailer?
When `Reviewers:` comes first, a following trailer is absorbed into the
`Reviewers:` value, as it is on trunk:
```
Reviewers: Ada Lovelace <[email protected]>
Generated-by: Codex (GPT-6)
```
becomes, both before and after this PR,
```
Reviewers: Ada Lovelace <[email protected]> Generated-by: Codex (GPT-6)
```
and `git interpret-trailers --parse` only finds one trailer.
With the opposite order, this PR is an improvement. On trunk neither trailer
parses, while after this PR both do. But a long `Reviewers:` line is then never
wrapped. This stays as a 118-character line:
```
Generated-by: Codex (GPT-6)
Reviewers: Ada Lovelace <[email protected]>, Grace Hopper
<[email protected]>, Katherine Johnson <[email protected]>
```
In both orders I'd expect both trailers to parse, with the `Reviewers:` line
wrapped and other trailer lines left as written.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
