release_notes: append #PR to markdown link text #11276

scpeters · 2021-04-29T04:25:19Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

I noticed in the release notes for 3.1.4 that some lines have an author name but no pull request. As of this writing, the release notes start with the following markdown code (reproduce with brew ruby -rrelease_notes -e 'puts ReleaseNotes.generate_release_notes("3.1.3", "3.1.4", markdown: true)'):

- [upgrade: fix broken dependents never being built from source](https://github.com/Homebrew/brew/pull/11263) (@Bo98)
- [Update maintainers, manpage and completions.](https://github.com/Homebrew/brew/pull/11261) (@Homebrew)
- [Allow man and prof Apple Silicon](https://github.com/Homebrew/brew/pull/11256) (@dtrodrigues)
- [Interesting-Taps-and-Forks.md: add gromgit/fuse](https://github.com/Homebrew/brew/pull/11255) (@gromgit)
- [docs: add Cask Cookbook](https://github.com/Homebrew/brew/pull/11249) (@SMillerDev)
- [dev-cmd/bottle: use gnu-tar universally, sort entries & use PAX](https://github.com/Homebrew/brew/pull/11253) (@Bo98)
- [](https://github.com/Homebrew/brew/pull/11129) (@SMillerDev)

Notice that the last line has a markdown link to #11129, but it is not rendered because it has no link text. I believe this is because the merge commit for that pull request afa99b4 has only one line of description, while other merge commits, such as 94ce323 for #11263, have an additional line of description.

There are 3 pull requests in 3.1.4 that have this issue. I don't know why these merge commits are like this. As a workaround, I've modified release_notes.rb to append an extra #11129 to the Markdown link text. With this change the beginning of the 3.1.4 release notes would have the following markdown:

- [upgrade: fix broken dependents never being built from source #11263](https://github.com/Homebrew/brew/pull/11263) (@Bo98)
- [Update maintainers, manpage and completions. #11261](https://github.com/Homebrew/brew/pull/11261) (@Homebrew)
- [Allow man and prof Apple Silicon #11256](https://github.com/Homebrew/brew/pull/11256) (@dtrodrigues)
- [Interesting-Taps-and-Forks.md: add gromgit/fuse #11255](https://github.com/Homebrew/brew/pull/11255) (@gromgit)
- [docs: add Cask Cookbook #11249](https://github.com/Homebrew/brew/pull/11249) (@SMillerDev)
- [dev-cmd/bottle: use gnu-tar universally, sort entries & use PAX #11253](https://github.com/Homebrew/brew/pull/11253) (@Bo98)
- [ #11129](https://github.com/Homebrew/brew/pull/11129) (@SMillerDev)

BrewTestBot · 2021-04-29T04:25:34Z

Review period will end on 2021-04-30 at 04:25:19 UTC.

Rylan12

Thanks for looking into this. When merging, the merger does have the option to modify the commit message I believe so it's possible that the people who merged removed that line that is missing. Although, I was the one who merged one of the missing ones and I'm almost positive I didn't touch the merge commit message so it's possible that there's a reason that line's being omitted that isn't coming to me.

I like this solution, though. I wonder, though, if it looks better to use this instead:

#11191: (@Rylan12)

#11192: workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

Or maybe bold (with or without the colon):

#11191: (@Rylan12)

#11192: workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

#11191 (@Rylan12)

#11192 workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

That way the PR number is always first so if the title is missing it's a little more consistent. Plus, it might make a little more sense to have the PR number first anyway.

Also, I don't think we need to rely on a single string interpolation to get this right. If we need to use some more complex logic to change the markdown line if the title is blank we definitely have the room to do so. It might also make it cleaner-looking. For example, only adding the colon and extra spaces if a title is present.

Rylan12 · 2021-04-29T05:44:39Z

Library/Homebrew/release_notes.rb

+        %r{(.*)/(\d+) \(@(.+)\) - (.*)} =~ s
+        "- [#{Regexp.last_match(4)} \##{Regexp.last_match(2)}](#{Regexp.last_match(1)}/#{Regexp.last_match(2)})" \
+        " (@#{Regexp.last_match(3)})\n"


Suggested change

%r{(.*)/(\d+) \(@(.+)\) - (.*)} =~ s

"- [#{Regexp.last_match(4)} \##{Regexp.last_match(2)}](#{Regexp.last_match(1)}/#{Regexp.last_match(2)})" \

" (@#{Regexp.last_match(3)})\n"

matches =~ s.match %r{(?<url>.*)/(?<pr>\d+) \(@(?<user>.+)\) - (?<title>.*)}

"- [#{matches[:title]} \##{matches[:pr]}](#{matches[:url]}/#{matches[:pr]})" (@#{matches[:user]})\n"

I think using named captures will help readability here

your suggestions sound good to me; I'm about to go to bed, so please feel free to push to this branch as you see fit

I'm about to go to bed

Same here (should have done that hours ago) 🛌😴💤

I'll take a look when I'm available tomorrow if you haven't had a chance yet.

MikeMcQuaid · 2021-04-29T13:56:27Z

Notice that the last line has a markdown link to #11129, but it is not rendered because it has no link text. I believe this is because the merge commit for that pull request afa99b4 has only one line of description, while other merge commits, such as 94ce323 for #11263, have an additional line of description.

I'd suggest we consider falling back to the first line of the merge commit if the second line isn't present. That at least provides a link to the PR and a branch name that may provide more context.

scpeters · 2021-04-29T16:15:31Z

Notice that the last line has a markdown link to #11129, but it is not rendered because it has no link text. I believe this is because the merge commit for that pull request afa99b4 has only one line of description, while other merge commits, such as 94ce323 for #11263, have an additional line of description.

I'd suggest we consider falling back to the first line of the merge commit if the second line isn't present. That at least provides a link to the PR and a branch name that may provide more context.

so it would look like this:

Merge pull request #11129 from SMillerDev/feature/audit/service_command (@SMillerDev)

scpeters · 2021-04-29T16:21:11Z

to summarize some of the proposed display options:

this PR as of 6e91c35:

from @Rylan12:

#11191: (@Rylan12)

#11192: workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

Or maybe bold (with or without the colon):

#11191: (@Rylan12)

#11192: workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

#11191 (@Rylan12)

#11192 workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

from @MikeMcQuaid

scpeters · 2021-04-29T16:22:35Z

here's a blend of those proposals that I like:

#11191: Merge pull request #11191 from Rylan12/re-enable-gitlab-patch-check (@Rylan12)
#11192: workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

MikeMcQuaid · 2021-04-29T16:27:36Z

Personally I think displaying the PR number is redundant if it's going to be link text.

Another option that occurred to me that'd be more involved would be to extract the PR number and query the title from the GitHub API.

Rylan12 · 2021-04-29T22:17:36Z

Yeah, I think I agree that including the number just adds extra text that isn't really that useful if there's already a link. I guess we could list the PR only if the description is blank:

#11191 (@Rylan12)

workflows/tests: simulate macOS for brew style homebrew-core (@Rylan12)

Another option that occurred to me that'd be more involved would be to extract the PR number and query the title from the GitHub API.

Yeah, I wouldn't be opposed to this. We already have the number so it shouldn't be that much more work. Plus it doesn't seem to happen that often so doing an API request every once in a while when brew release is run seems fine. If we really wanted we could base the release notes purely off of the titles from the API but that's probably an unnecessary number of queries for the same result.

BrewTestBot · 2021-04-30T06:02:08Z

Review period ended.

scpeters · 2021-05-03T07:12:11Z

I just added a new test case in fe994a3 for the behavior when a merge commit has a subject message but no body message. Let's use that new test expectation to capture the desired formatting

scpeters · 2021-05-03T07:42:59Z

I made a quick attempt at updating the regular expression code, but I had some trouble. I won't be able to work on this for a few days.

MikeMcQuaid · 2021-05-03T11:03:47Z

Library/Homebrew/test/release_notes_spec.rb

    end
  end

  describe ".generate_release_notes" do
    it "generates release notes" do
+      # rubocop:disable Style/TrailingWhitespace


Seems undesirable to have trailing whitespace generated here?

that was a placeholder, and it's gone now

MikeMcQuaid · 2021-05-03T11:04:01Z

Library/Homebrew/test/release_notes_spec.rb

    end

    it "generates markdown release notes" do
      expect(described_class.generate_release_notes("release-notes-testing", "HEAD", markdown: true)).to eq <<~NOTES
-        - [Do something else](https://github.com/Homebrew/brew/pull/2) (@User)
-        - [Do something](https://github.com/Homebrew/brew/pull/1) (@Homebrew)
+        - [ #3](https://github.com/Homebrew/brew/pull/3) (@User)


Similarly this output seems a bit weird. How about Merge pull request #3 from User/another_change or anything relating to another_change (as it's a branch name)?

I've used the subject line of the commit if the body is empty (so Merge pull request #3 from User/another_change)

scpeters · 2021-05-07T04:46:57Z

I reverted my changes with the PR number but kept the new test case and added code based on @MikeMcQuaid's suggestion to use the subject line of the commit message if the body is missing. I'll leave the approach using the GitHub API for another day.

I rebased to clean this up but the commits aren't verified. I tried following the gpg signing guide on my Linux machine, but it's not currently working (my previous commits were verified because I committed through the web browser). I can try to configure my mac with gpg signing and re-push if needed

MikeMcQuaid

Looks great, nice work here!

Some merge commits don't include the title of the pull request on subsequent lines, leaving the markdown link text blank in the release notes. This appends the subject line of the commit message instead if the body is missing and adds a test case.

scpeters requested a review from Rylan12 April 29, 2021 04:25

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 29, 2021

Rylan12 reviewed Apr 29, 2021

View reviewed changes

BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 30, 2021

BrewTestBot previously approved these changes Apr 30, 2021

View reviewed changes

scpeters dismissed BrewTestBot’s stale review via fe994a3 May 3, 2021 07:10

BrewTestBot previously approved these changes May 3, 2021

View reviewed changes

MikeMcQuaid reviewed May 3, 2021

View reviewed changes

scpeters dismissed BrewTestBot’s stale review via 371d679 May 7, 2021 04:42

scpeters force-pushed the release_notes_fix_blank_link branch from fe994a3 to 371d679 Compare May 7, 2021 04:42

BrewTestBot previously approved these changes May 7, 2021

View reviewed changes

scpeters dismissed BrewTestBot’s stale review via 944ede4 May 7, 2021 04:49

scpeters force-pushed the release_notes_fix_blank_link branch from 371d679 to 944ede4 Compare May 7, 2021 04:49

BrewTestBot previously approved these changes May 7, 2021

View reviewed changes

MikeMcQuaid previously approved these changes May 7, 2021

View reviewed changes

Rylan12 previously approved these changes May 7, 2021

View reviewed changes

MikeMcQuaid enabled auto-merge May 10, 2021 15:11

MikeMcQuaid dismissed Rylan12’s stale review via f2c46d4 May 10, 2021 15:30

MikeMcQuaid dismissed stale reviews from BrewTestBot and themself via f2c46d4 May 10, 2021 15:30

MikeMcQuaid force-pushed the release_notes_fix_blank_link branch from 944ede4 to f2c46d4 Compare May 10, 2021 15:30

BrewTestBot approved these changes May 10, 2021

View reviewed changes

Rylan12 approved these changes May 10, 2021

View reviewed changes

MikeMcQuaid merged commit c92a40a into Homebrew:master May 10, 2021

scpeters deleted the release_notes_fix_blank_link branch May 10, 2021 17:21

github-actions bot added the outdated PR was locked due to age label Jun 10, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

release_notes: append #PR to markdown link text #11276

release_notes: append #PR to markdown link text #11276

scpeters commented Apr 29, 2021 •

edited

Loading

BrewTestBot commented Apr 29, 2021

Rylan12 left a comment •

edited

Loading

Rylan12 Apr 29, 2021

scpeters Apr 29, 2021

Rylan12 Apr 29, 2021

MikeMcQuaid commented Apr 29, 2021

scpeters commented Apr 29, 2021

scpeters commented Apr 29, 2021

scpeters commented Apr 29, 2021

MikeMcQuaid commented Apr 29, 2021

Rylan12 commented Apr 29, 2021

BrewTestBot commented Apr 30, 2021

scpeters commented May 3, 2021

scpeters commented May 3, 2021

MikeMcQuaid May 3, 2021

scpeters May 7, 2021

MikeMcQuaid May 3, 2021

scpeters May 7, 2021

scpeters commented May 7, 2021

MikeMcQuaid left a comment

release_notes: append #PR to markdown link text #11276

release_notes: append #PR to markdown link text #11276

Conversation

scpeters commented Apr 29, 2021 • edited Loading

BrewTestBot commented Apr 29, 2021

Rylan12 left a comment • edited Loading

Choose a reason for hiding this comment

Rylan12 Apr 29, 2021

Choose a reason for hiding this comment

scpeters Apr 29, 2021

Choose a reason for hiding this comment

Rylan12 Apr 29, 2021

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 29, 2021

scpeters commented Apr 29, 2021

scpeters commented Apr 29, 2021

scpeters commented Apr 29, 2021

MikeMcQuaid commented Apr 29, 2021

Rylan12 commented Apr 29, 2021

BrewTestBot commented Apr 30, 2021

scpeters commented May 3, 2021

scpeters commented May 3, 2021

MikeMcQuaid May 3, 2021

Choose a reason for hiding this comment

scpeters May 7, 2021

Choose a reason for hiding this comment

MikeMcQuaid May 3, 2021

Choose a reason for hiding this comment

scpeters May 7, 2021

Choose a reason for hiding this comment

scpeters commented May 7, 2021

MikeMcQuaid left a comment

Choose a reason for hiding this comment

scpeters commented Apr 29, 2021 •

edited

Loading

Rylan12 left a comment •

edited

Loading