dongjoon-hyun commented on code in PR #59708:
URL: https://github.com/apache/spark/pull/59708#discussion_r4362249962
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but
breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading
id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
+
+
+def _leading_spark_brackets(title: str) -> List[str]:
+ """Collect the run of leading ``[SPARK-N]`` bracket tags from ``title``.
+
+ Used as a fallback when :meth:`Title.parse` rejects a title that still
+ begins with one or more ``[SPARK-N]`` tags. Scans from the start and stops
+ at the first bracket that is not a SPARK id, so a SPARK id in the body is
+ not collected.
+
+ >>> _leading_spark_brackets("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> _leading_spark_brackets("[SPARK-1][SPARK-2][SQL] Fix mentions
SPARK-9999")
+ ['SPARK-1', 'SPARK-2']
+ >>> _leading_spark_brackets("[SQL] no leading id")
+ []
+ """
+ ids = []
+ pos = 0
Review Comment:
Minor: `Title.parse` strips the title first (`raw = raw.strip()`, L2119),
but this scan starts at position 0 of the unstripped title, so the two paths in
`leading_jira_ids` disagree on leading whitespace. At 4c11086,
`leading_jira_ids('Revert " [SPARK-1][SQL] Fix"')` returns `['SPARK-1']`
because the parse succeeds, but `leading_jira_ids('Revert "
[SPARK-1][SQL][MINOR] Fix"')` returns `[]` because the parse fails and the
fallback finds no bracket at position 0. Could you strip the title before
scanning, as `Title.parse` does?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but
breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading
id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
Review Comment:
**Wrapped titles whose id is not in a leading bracket still lose it.** The
fallback only collects `[SPARK-N]` brackets that start at position 0, so a bare
leading id (`SPARK-N: ...`) or a tag before the id (`[WIP] [SPARK-N]`) yields
`[]`, while the old `re.findall` found it. There is a recent example in
master's history: c8db813644f has the subject `SPARK-49004`, and its revert
f15570dbfe1 (2024-08-01) is `Revert "SPARK-49004"`. I ran this helper at
4c11086: `leading_jira_ids('Revert "SPARK-49004"')` returns `[]`, while
master's regex returns `['SPARK-49004']`. For such a revert, `main()` skips the
Epic/Umbrella blocker and the match prompt, and `update_jira_for_pr` calls
`revert_jira_issue(branches, "")`, so pressing Enter skips the fix-version
removal. Of the 617 wrapped `Revert "..."` / `Reapply "..."` subjects in
master's history, 10 lose their only id this way; the other 9 are from
2014-2015. Could you also accept a bare `SPARK-N` at the start of the unwrapped
title, or say i
n the docstring that such titles get no default id?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but
breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading
id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
Review Comment:
Nit: for every title that `Title.parse` accepts, `Title.parse(t).jira_ids()`
equals `_leading_spark_brackets(t.strip())`. The parse rules require SPARK ids
to come before any component and do not let a marker coexist with an id, so the
accepted ids are always the first run of `[SPARK-N]` brackets. I compared both
on all 50,682 subjects in master's history and found no difference. So this
`try`/`except`, and `Title.jira_ids`, whose only caller is this function, are a
second path to the same result. Could you use `return
_leading_spark_brackets(_unwrap_title(title).strip())` and drop
`Title.jira_ids`? That would also fix the whitespace case on L2266.
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
Review Comment:
Nit: my item 9 is still partly open. The `re.findall` comparison it asked to
drop was in this docstring (L2187 at 85aa4ba), and it is still here; the reply
says it was dropped from `Title.jira_ids`, which never had it. Two of the three
`Title.jira_ids` doctests (L2161 and L2165) also still repeat
`leading_jira_ids` inputs. Could you drop this sentence and the repeated
examples?
##########
dev/merge_spark_pr.py:
##########
@@ -1888,7 +1888,10 @@ def resolve_jira_issues(title, merge_branches, comment,
title_components=()):
def update_jira_for_pr(pr_num, title, merge_branches, title_components,
is_revert=False):
skip_jira_title_tags = ("MINOR", "TRIVIAL", "FOLLOWUP")
tags = set(title_components)
- jira_title = title[len('Revert "') : -1] if is_revert else title
+ # Revert/Reapply PRs keep their title verbatim, so unwrap before parsing
to see the
Review Comment:
Nit: this comment says the title is unwrapped to see the wrapped id, but
`jira_title` only feeds the tag set below. The ids come from
`leading_jira_ids(title)` on L1894, which gets the still-wrapped `title` and
unwraps it again. Could you pass `jira_title` on L1894, or say in the comment
that only the markers and components come from `jira_title`?
##########
dev/merge_spark_pr.py:
##########
@@ -2853,6 +2975,16 @@ def main():
... resolver.assert_not_called()
... confirm.assert_not_called()
+ Nested wrappers are unwrapped before the skip-tag and id checks, so an
inner
+ skip tag is still honored and an inner id still reaches the revert path.
+
+ >>> update('Revert "Revert "[MINOR][SQL] Fix""', ["master"],
is_revert=True)
Review Comment:
Minor: this PR also changes what happens to `Reapply` titles with a skip
tag. On master, `update_jira_for_pr` unwrapped only `Revert "`, so `Reapply
"[MINOR][SQL] Fix"` reached `resolve_jira_issues`; now it is unwrapped and
skipped. I ran `update_jira_for_pr` at 4c11086 with `Reapply "[MINOR][SQL]
Fix"` and `Reapply "[SPARK-1234][SQL][FOLLOWUP] Fix"`: neither
`resolve_jira_issues` nor `continue_maybe` is called. But the skip-tag doctests
(L2969-L2976 and here) use only `Revert "..."` titles with `is_revert=True`, so
nothing covers this new path. Could you add the two `Reapply` titles, called
without `is_revert`, to the skip-tag loop?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
+ while changed:
+ changed = False
+ for prefix in ('Revert "', 'Reapply "'):
+ if title.startswith(prefix) and title.endswith('"'):
+ title = title[len(prefix) : -1]
+ changed = True
+ break
+ return title
+
+
+def leading_jira_ids(title: str) -> List[str]:
+ """Return the leading SPARK-NNNNN JIRA ids of ``title``, in order.
+
+ Unlike a bare ``re.findall("SPARK-[0-9]{4,5}", title)``, only ids that
+ appear as leading bracket tags are returned, so a SPARK-NNNNN reference in
+ the title body is not mistaken for the ticket the PR resolves. Any
+ ``Revert "..."`` / ``Reapply "..."`` wrappers are stripped first (including
+ nested ones), so those PRs still yield the wrapped id.
+
+ The title is parsed with :meth:`Title.parse`. When parsing fails -- the
+ title has no recognized leading tag, or starts with ``[SPARK-N]`` but
breaks
+ a later rule (e.g. ``[SPARK-1234][SQL][MINOR] Fix``) -- the leading
+ ``[SPARK-N]`` brackets are still collected directly, so a clearly leading
id
+ is never dropped. A title with no leading ``[SPARK-N]`` bracket yields an
+ empty list.
+
+ >>> leading_jira_ids("[SPARK-1234][SQL] Follow up to SPARK-5678")
+ ['SPARK-1234']
+ >>> leading_jira_ids('Reapply "[SPARK-1234][SQL] Fix"')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ ['SPARK-1234']
+ >>> leading_jira_ids("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> leading_jira_ids("[MINOR][SQL] Fix typo")
+ []
+ >>> leading_jira_ids("No leading tag, mentions SPARK-5678")
+ []
+ """
+ title = _unwrap_title(title)
+ try:
+ return Title.parse(title).jira_ids()
+ except ValueError:
+ return _leading_spark_brackets(title)
+
+
+def _leading_spark_brackets(title: str) -> List[str]:
+ """Collect the run of leading ``[SPARK-N]`` bracket tags from ``title``.
+
+ Used as a fallback when :meth:`Title.parse` rejects a title that still
+ begins with one or more ``[SPARK-N]`` tags. Scans from the start and stops
+ at the first bracket that is not a SPARK id, so a SPARK id in the body is
+ not collected.
+
+ >>> _leading_spark_brackets("[SPARK-1234][SQL][MINOR] Fix")
+ ['SPARK-1234']
+ >>> _leading_spark_brackets("[SPARK-1][SPARK-2][SQL] Fix mentions
SPARK-9999")
+ ['SPARK-1', 'SPARK-2']
+ >>> _leading_spark_brackets("[SQL] no leading id")
+ []
+ """
+ ids = []
+ pos = 0
+ while pos < len(title):
+ m = _BRACKET_TAG_RE.match(title, pos)
Review Comment:
Nit: this loop copies the bracket scan in `Title.parse` (L2126-L2145):
`_BRACKET_TAG_RE.match(raw, pos)`, `.upper()`, `pos = m.end()`, and the inner
loop that skips spaces. If `Title.parse` later accepts another separator
between tags, this copy keeps the old rule. Could both use one small generator
that yields each leading tag?
##########
dev/merge_spark_pr.py:
##########
@@ -2156,6 +2175,109 @@ def __str__(self) -> str:
return parts + (" " if parts else "") + self.text
+def _unwrap_title(title: str) -> str:
+ """Strip ``Revert "..."`` / ``Reapply "..."`` wrappers from a PR title.
+
+ ``main()`` keeps Revert and Reapply PR titles verbatim, so the JIRA id,
+ leading markers, and components all live inside the quotes. Unwrapping
here
+ lets the id-extraction and skip-tag logic see them. Wrappers are stripped
+ repeatedly, so a revert-of-revert (``Revert "Revert "...""``), including
+ mixed ``Revert``/``Reapply`` nesting, is reduced to its innermost title. A
+ title without such a wrapper is returned unchanged.
+
+ >>> _unwrap_title('Revert "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Reapply "[SPARK-1234][SQL] Fix"')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Revert "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "Reapply "[SPARK-1234][SQL] Fix""')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('[SPARK-1234][SQL] Fix')
+ '[SPARK-1234][SQL] Fix'
+ >>> _unwrap_title('Revert "')
+ ''
+ """
+ changed = True
Review Comment:
Nit: the `changed` flag, the `for` loop, and the `break` could be one regex
next to the other `_*_RE` constants, e.g. `_WRAPPER_RE =
re.compile(r'(?:Revert|Reapply) "(.*)"')` with `while (m :=
_WRAPPER_RE.fullmatch(title)): title = m.group(1)`. I checked it against the
doctests above; only the degenerate `Revert "` case changes, returning the
input instead of `''`. Could you simplify it that way?
--
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]
