tools/changelog-helper: Ignore non-MERGED PRs & ignore already listed PRs faster #2485
Conversation
tools/changelog-helper.sh
Outdated
| @@ -126,10 +126,15 @@ check_or_add_pr() { | |||
| return | |||
| fi | |||
| checked_ids[$id]=1 | |||
| local json=$(gh pr view "${id/#/}" --json title,author) | |||
| if grep -qF "#$id" <<<"$changelog"; then | |||
There was a problem hiding this comment.
That will find #300 if the list contains #3000 through to #3009. A more precise match would be good.
There was a problem hiding this comment.
Good point. Will probably add \b or something around that later.
tools/changelog-helper.sh
Outdated
| @@ -126,7 +126,7 @@ check_or_add_pr() { | |||
| return | |||
| fi | |||
| checked_ids[$id]=1 | |||
| if grep -qF "#$id" <<<"$changelog"; then | |||
| if grep -qP "#$id([^0-9]|$)" <<<"$changelog"; then | |||
There was a problem hiding this comment.
Doesn't "#$id\>" work? I don't think I'd want it matching #12AB, either. (Think commit IDs.)
There was a problem hiding this comment.
Doesn't
"#$id\>"work?
I didn't know about that -- thanks! It works in -E mode (maybe \b might have worked there, too). Updated.
There was a problem hiding this comment.
I think \b will only work in -P (PCRE) mode, if the installed version of grep supports it. I prefer \< and \> if I'm not actually writing perl or PHP.
jamulussoftware#2485 (comment) Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>
hoffie
force-pushed
the
changelog-helper-MERGED-only
branch
from
68f6b7b to
b4dde92
Compare
| @@ -126,10 +126,15 @@ check_or_add_pr() { | |||
| return | |||
| fi | |||
| checked_ids[$id]=1 | |||
| local json=$(gh pr view "${id/#/}" --json title,author) | |||
| if grep -qE "#$id\>" <<<"$changelog"; then | |||
There was a problem hiding this comment.
Can you add a comment on what this does and why, please.
There was a problem hiding this comment.
And, perhaps - like the "not merged" below - echo a message.
There was a problem hiding this comment.
Can you add a comment on what this does and why, please.
Done.
And, perhaps - like the "not merged" below - echo a message.
Hm, I'm not convinced. It was generally nice to see that repeated invocations of this script added less and less stuff (e.g. when run again later in the release process). If we generate a line of output of each PR even if it was already there, the output becomes harder to read, IMO. If you feel strongly, I can add it though.
There was a problem hiding this comment.
Yeah, that's why I was hesitant. Probably best without.
jamulussoftware#2485 (comment) Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>
hoffie
force-pushed
the
changelog-helper-MERGED-only
branch
from
b4dde92 to
e92000a
Compare
jamulussoftware#2485 (comment) Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>
Short description of changes
tools/changelog-helper: Ignore non-MERGED PRs
tools/changelog-helper: Ignore already listed PRs faster
CHANGELOG: Internal: Improved Changelog release tooling.
Context: Fixes an issue?
#1865 (comment)
Does this change need documentation? What needs to be documented and how?
Status of this Pull Request
Ready.
What is missing until this pull request can be merged?
Ready.
Checklist