dongjoon-hyun commented on code in PR #60031:
URL: https://github.com/apache/spark/pull/60031#discussion_r4448436183
##########
dev/merge_spark_pr.py:
##########
@@ -2283,6 +2285,24 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_wrappers(title: str) -> tuple[str, list[str]]:
+ """Return the innermost title and the wrapper words
(``Revert``/``Reapply``), outermost first.
+
+ See :func:`_unwrap_title` for how the wrappers are stripped.
+
+ >>> _unwrap_wrappers('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ('[SPARK-1234][SQL] Fix', ['Revert', 'Reapply'])
+ >>> _unwrap_wrappers('[SPARK-1234][SQL] Fix')
+ ('[SPARK-1234][SQL] Fix', [])
+ """
+ title = title.strip()
+ words = []
+ while m := _WRAPPER_RE.fullmatch(title):
Review Comment:
**[1] Unbalanced nested quotes fall back to the outermost wrapper.**
A nested title with one closing quote missing is still classified by the
layers that do balance, so the parity check silently falls back to the old
outermost-only decision.
For example, a hand-edited re-land titled `Revert "Revert "[SPARK-N][SQL]
X"` (one `"` dropped): the outer layer matches, but the inner `Revert
"[SPARK-N][SQL] X` does not end with `"`, so `words == ['Revert']` and
`update_jira_for_pr` takes the revert path. The leftover title also does not
start with a bracket, so `leading_jira_ids` returns `[]` and the committer gets
an empty JIRA-id prompt, and an inner `[MINOR]` would not be honored either.
Since the direction now depends on the full nesting, could `main()` fail (or
at least warn) when the innermost title still starts with a wrapper word?
Something like:
```python
inner, words = _unwrap_wrappers(title)
if words and inner.startswith(('Revert "', 'Reapply "')):
fail("Unbalanced Revert/Reapply quotes in PR title: %s" % title)
```
##########
dev/merge_spark_pr.py:
##########
@@ -1994,6 +1994,8 @@ def update_jira_for_pr(pr_num, title, merge_branches,
title_components, is_rever
# initialize_jira() fails fast on a missing library/credentials when JIRA
is live.
print()
continue_maybe("Would you like to update an associated JIRA?")
+ # A revert of the innermost change, decided by all the wrappers (see
classify_wrapped_title).
+ _, is_revert = classify_wrapped_title(title)
Review Comment:
**[2] The direction is only known after the push.**
This one is not introduced by this PR, so feel free to leave it for a
separate JIRA.
The direction is decided here, after the merge has been pushed. Before the
push, `check_linked_jira_issues` still shows every wrapped title as a fresh
merge:
- For `Revert "[SPARK-N] X"` it prints `WARNING: already Resolved/Closed`,
which is the expected state for a revert.
- For `Revert "Revert "[SPARK-N] X""` nothing tells the committer that the
script will resolve rather than revert.
Deciding the direction once in `main()`, showing it in the pre-merge
summary, and passing it here would let the committer catch a wrong direction
before the irreversible step.
##########
dev/merge_spark_pr.py:
##########
@@ -2138,7 +2140,7 @@ def normalize(cls, token):
# A SPARK id that opens a title without brackets, e.g. the old subject
"SPARK-49004".
_BARE_SPARK_ID_RE = re.compile(r"SPARK-[0-9]+\b", re.IGNORECASE | re.ASCII)
# A Revert "..." / Reapply "..." wrapper, which main() keeps verbatim in the
PR title.
Review Comment:
**[4] "Verbatim" is no longer accurate.**
This PR strips surrounding whitespace from wrapped titles and changes two
comments in `main()` from "verbatim" to "as is", but a few other places still
say that `main()` keeps them verbatim:
- this comment
- L1975 in `update_jira_for_pr`
- L2309 in the `_unwrap_title` docstring
- L1165 in the `title_similarity` docstring
Now that `' Revert "[SPARK-N] X" '` is committed as `Revert "[SPARK-N] X"`,
could these be updated too?
##########
dev/merge_spark_pr.py:
##########
@@ -2307,10 +2327,56 @@ def _unwrap_title(title: str) -> str:
>>> _unwrap_title('Revert "')
'Revert "'
"""
- title = title.strip()
- while m := _WRAPPER_RE.fullmatch(title):
- title = m.group(1).strip()
- return title
+ return _unwrap_wrappers(title)[0]
+
+
+def classify_wrapped_title(title: str) -> tuple[bool, bool]:
+ """Return ``(is_wrapped, is_revert)`` for a PR title.
+
+ ``is_wrapped`` is true when the title has a ``Revert "..."`` / ``Reapply
"..."``
+ wrapper (see :func:`_unwrap_title`); ``main()`` keeps such titles as they
are,
+ apart from surrounding whitespace.
+ ``is_revert`` is true when the wrappers net to a revert of the innermost
change,
+ which ``update_jira_for_pr`` handles by removing fix versions instead of
resolving.
+ The direction is decided by all the wrappers, not only the outermost one:
each
+ ``Revert`` flips it, while a ``Reapply``, which git uses for a revert of a
revert,
+ flips it twice and so leaves it unchanged. So the title is a revert when
it has an
+ odd number of ``Revert`` wrappers.
+
+ >>> for t in ('[SPARK-1234][SQL] Fix',
+ ... 'Revert "[SPARK-1234][SQL] Fix"',
+ ... 'Reapply "[SPARK-1234][SQL] Fix"',
+ ... 'Revert "Revert "[SPARK-1234][SQL] Fix""',
+ ... 'Revert "Reapply "[SPARK-1234][SQL] Fix""',
+ ... 'Reapply "Revert "[SPARK-1234][SQL] Fix""',
+ ... 'Reapply "Reapply "[SPARK-1234][SQL] Fix""'):
+ ... print(classify_wrapped_title(t), t)
+ (False, False) [SPARK-1234][SQL] Fix
+ (True, True) Revert "[SPARK-1234][SQL] Fix"
+ (True, False) Reapply "[SPARK-1234][SQL] Fix"
+ (True, False) Revert "Revert "[SPARK-1234][SQL] Fix""
+ (True, True) Revert "Reapply "[SPARK-1234][SQL] Fix""
+ (True, True) Reapply "Revert "[SPARK-1234][SQL] Fix""
+ (True, False) Reapply "Reapply "[SPARK-1234][SQL] Fix""
+
+ Three levels of nesting, which a doctest in a docstring cannot spell
literally:
+
+ >>> q = '"'
+ >>> classify_wrapped_title('Revert "Revert "Revert "[SPARK-1234] Fix' + q
* 3)
+ (True, True)
+ >>> classify_wrapped_title('Revert "Reapply "Revert "[SPARK-1234] Fix' + q
* 3)
+ (True, False)
+
+ Surrounding whitespace is ignored, as in :func:`_unwrap_title`. A bare
``Revert "``
+ is not a wrapper, so ``main()`` parses it as an ordinary title and rejects
it.
+
+ >>> classify_wrapped_title(' Revert "[SPARK-1234][SQL] Fix" ')
+ (True, True)
+ >>> classify_wrapped_title('Revert "'), classify_wrapped_title('Reapply "')
+ ((False, False), (False, False))
+ """
+ _, words = _unwrap_wrappers(title)
Review Comment:
**[3] Each caller discards half of the tuple.**
`main()` keeps only `is_wrapped` (`is_wrapped_pr, _ = ...`) and
`update_jira_for_pr` keeps only `is_revert` (`_, is_revert = ...`). A tuple
where every caller drops one element, next to `_unwrap_title` and
`_unwrap_wrappers`, is more API than needed. How about a single predicate?
```python
def _is_revert_title(title: str) -> bool:
return _unwrap_wrappers(title)[1].count("Revert") % 2 == 1
```
`main()` could then use `bool(_unwrap_wrappers(title)[1])`, and the parity
explanation in this docstring would move to the predicate.
##########
dev/merge_spark_pr.py:
##########
@@ -2307,10 +2327,56 @@ def _unwrap_title(title: str) -> str:
>>> _unwrap_title('Revert "')
'Revert "'
"""
- title = title.strip()
- while m := _WRAPPER_RE.fullmatch(title):
- title = m.group(1).strip()
- return title
+ return _unwrap_wrappers(title)[0]
+
+
+def classify_wrapped_title(title: str) -> tuple[bool, bool]:
+ """Return ``(is_wrapped, is_revert)`` for a PR title.
+
+ ``is_wrapped`` is true when the title has a ``Revert "..."`` / ``Reapply
"..."``
+ wrapper (see :func:`_unwrap_title`); ``main()`` keeps such titles as they
are,
+ apart from surrounding whitespace.
+ ``is_revert`` is true when the wrappers net to a revert of the innermost
change,
+ which ``update_jira_for_pr`` handles by removing fix versions instead of
resolving.
+ The direction is decided by all the wrappers, not only the outermost one:
each
+ ``Revert`` flips it, while a ``Reapply``, which git uses for a revert of a
revert,
+ flips it twice and so leaves it unchanged. So the title is a revert when
it has an
+ odd number of ``Revert`` wrappers.
+
+ >>> for t in ('[SPARK-1234][SQL] Fix',
+ ... 'Revert "[SPARK-1234][SQL] Fix"',
+ ... 'Reapply "[SPARK-1234][SQL] Fix"',
+ ... 'Revert "Revert "[SPARK-1234][SQL] Fix""',
+ ... 'Revert "Reapply "[SPARK-1234][SQL] Fix""',
+ ... 'Reapply "Revert "[SPARK-1234][SQL] Fix""',
+ ... 'Reapply "Reapply "[SPARK-1234][SQL] Fix""'):
+ ... print(classify_wrapped_title(t), t)
+ (False, False) [SPARK-1234][SQL] Fix
+ (True, True) Revert "[SPARK-1234][SQL] Fix"
+ (True, False) Reapply "[SPARK-1234][SQL] Fix"
+ (True, False) Revert "Revert "[SPARK-1234][SQL] Fix""
+ (True, True) Revert "Reapply "[SPARK-1234][SQL] Fix""
+ (True, True) Reapply "Revert "[SPARK-1234][SQL] Fix""
+ (True, False) Reapply "Reapply "[SPARK-1234][SQL] Fix""
+
+ Three levels of nesting, which a doctest in a docstring cannot spell
literally:
Review Comment:
**[6] Three-level titles can be spelled directly.**
This docstring is not raw, so escaped quotes put `"""` into the doctest
source, and the three-level cases can be written like the others:
```python
>>> classify_wrapped_title('Revert "Revert "Revert "[SPARK-1234]
Fix\"\"\"')
(True, True)
```
I checked that a doctest written this way runs. That would drop the `q`
helper and this note.
##########
dev/merge_spark_pr.py:
##########
@@ -2652,23 +2718,25 @@ def main():
pr_events = get_json("%s/issues/%s/events" % (GITHUB_API_BASE, pr_num))
url = pr["url"]
- title = pr["title"]
+ # Strip surrounding whitespace, which classify_wrapped_title() ignores, so
that a wrapped
+ # title, kept as is below, is committed without it.
+ title = pr["title"].strip()
Review Comment:
**[5] The `main()` changes have no test.**
The strip here, the `is_wrapped_pr` switch, and moving the `Normalized
title` print out of the `if not is_wrapped_pr` block are all in `main()`, which
the doctests don't reach (as the PR description notes). A regression such as
classifying `pr["title"]` instead of the stripped title, or re-indenting the
print, would pass every doctest.
Moving the title handling into a small helper (PR title in, `(title,
title_components)` out) would let a doctest pin the whitespace-wrapped and bare
`Revert "` cases that the description promises.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
