fix: improve release notes (#16343)

* fix: use version name in release notes

* fix: omit previously-released notes

* fix: sniff semantic commit types from PR subjects

instead of only from commit messages

* fix: do not use unrecognized semantic commit types

* chore: do not hardcode Release-Notes comment text

It used to be '<!-- One-line Change Summary Here-->',
it's currently a link to a best-practices page, and
it'll probably change again in the future. Let's just
match on <!--.*--> instead.

* chore: copyedit the help page

* chore: use clerk's OMIT_FROM_RELEASE_NOTES_KEYS

* chore: tweak comments

* chore: rename 'breaks' property as 'breaking'

script/prepare-release.js

@@ -50,12 +50,12 @@ async function getNewVersion (dryRun) {
   }
 }
 
-async function getReleaseNotes (currentBranch) {
+async function getReleaseNotes (currentBranch, newVersion) {
   if (bumpType === 'nightly') {
     return { text: 'Nightlies do not get release notes, please compare tags for info.' }
   }
   console.log(`Generating release notes for ${currentBranch}.`)
-  const releaseNotes = await releaseNotesGenerator(currentBranch)
+  const releaseNotes = await releaseNotesGenerator(currentBranch, newVersion)
   if (releaseNotes.warning) {
     console.warn(releaseNotes.warning)
   }
@@ -63,8 +63,8 @@ async function getReleaseNotes (currentBranch) {
 }
 
 async function createRelease (branchToTarget, isBeta) {
-  const releaseNotes = await getReleaseNotes(branchToTarget)
   const newVersion = await getNewVersion()
+  const releaseNotes = await getReleaseNotes(branchToTarget, newVersion)
   await tagRelease(newVersion)
 
   console.log(`Checking for existing draft release.`)
@@ -194,7 +194,8 @@ async function prepareRelease (isBeta, notesOnly) {
   } else {
     const currentBranch = (args.branch) ? args.branch : await getCurrentBranch(gitDir)
     if (notesOnly) {
-      const releaseNotes = await getReleaseNotes(currentBranch)
+      const newVersion = await getNewVersion(true)
+      const releaseNotes = await getReleaseNotes(currentBranch, newVersion)
       console.log(`Draft release notes are: \n${releaseNotes.text}`)
     } else {
       const changes = await changesToRelease(currentBranch)

script/release-notes/index.js

@@ -1,6 +1,7 @@
 #!/usr/bin/env node
 
 const { GitProcess } = require('dugite')
+const minimist = require('minimist')
 const path = require('path')
 const semver = require('semver')
 
@@ -120,13 +121,17 @@ const getPreviousPoint = async (point) => {
   }
 }
 
-async function getReleaseNotes (range, explicitLinks) {
+async function getReleaseNotes (range, newVersion, explicitLinks) {
   const rangeList = range.split('..') || ['HEAD']
   const to = rangeList.pop()
   const from = rangeList.pop() || (await getPreviousPoint(to))
-  console.log(`Generating release notes between ${from} and ${to}`)
 
-  const notes = await notesGenerator.get(from, to)
+  if (!newVersion) {
+    newVersion = to
+  }
+
+  console.log(`Generating release notes between ${from} and ${to} for version ${newVersion}`)
+  const notes = await notesGenerator.get(from, to, newVersion)
   const ret = {
     text: notesGenerator.render(notes, explicitLinks)
   }
@@ -139,15 +144,33 @@ async function getReleaseNotes (range, explicitLinks) {
 }
 
 async function main () {
-  // TODO: minimist/commander
+  const opts = minimist(process.argv.slice(2), {
-  const explicitLinks = process.argv.slice(2).some(arg => arg === '--explicit-links')
+    boolean: [ 'explicit-links', 'help' ],
-  if (process.argv.length > 4) {
+    string: [ 'version' ]
-    console.log('Use: script/release-notes/index.js [--explicit-links] [tag | tag1..tag2]')
+  })
-    return 1
+  opts.range = opts._.shift()
+  if (opts.help || !opts.range) {
+    const name = path.basename(process.argv[1])
+    console.log(`
+easy usage: ${name} version
+
+full usage: ${name} [begin..]end [--version version] [--explicit-links]
+
+ * 'begin' and 'end' are two git references -- tags, branches, etc --
+   from which the release notes are generated.
+ * if omitted, 'begin' defaults to the previous tag in end's branch.
+ * if omitted, 'version' defaults to 'end'. Specifying a version is
+   useful if you're making notes on a new version that isn't tagged yet.
+ * 'explicit-links' makes every note's issue, commit, or pull an MD link
+
+For example, these invocations are equivalent:
+  ${process.argv[1]} v4.0.1
+  ${process.argv[1]} v4.0.0..v4.0.1 --version v4.0.1
+`)
+    return 0
   }
 
-  const range = process.argv[2] || 'HEAD'
+  const notes = await getReleaseNotes(opts.range, opts.version, opts['explicit-links'])
-  const notes = await getReleaseNotes(range, explicitLinks)
   console.log(notes.text)
   if (notes.warning) {
     throw new Error(notes.warning)

script/release-notes/notes.js

@@ -63,6 +63,18 @@ const setPullRequest = (commit, owner, repo, number) => {
   }
 }
 
+// copied from https://github.com/electron/clerk/blob/master/src/index.ts#L4-L13
+const OMIT_FROM_RELEASE_NOTES_KEYS = [
+  'no-notes',
+  'no notes',
+  'no_notes',
+  'none',
+  'no',
+  'nothing',
+  'empty',
+  'blank'
+]
+
 const getNoteFromBody = body => {
   if (!body) {
     return null
@@ -76,21 +88,15 @@ const getNoteFromBody = body => {
     .find(paragraph => paragraph.startsWith(NOTE_PREFIX))
 
   if (note) {
-    const placeholder = '<!-- One-line Change Summary Here-->'
     note = note
       .slice(NOTE_PREFIX.length)
-      .replace(placeholder, '')
+      .replace(/<!--.*-->/, '') // '<!-- change summary here-->'
       .replace(/\r?\n/, ' ') // remove newlines
       .trim()
   }
 
-  if (note) {
+  if (note && OMIT_FROM_RELEASE_NOTES_KEYS.includes(note.toLowerCase())) {
-    if (note.match(/^[Nn]o[ _-][Nn]otes\.?$/)) {
+    return NO_NOTES
-      return NO_NOTES
-    }
-    if (note.match(/^[Nn]one\.?$/)) {
-      return NO_NOTES
-    }
   }
 
   return note
@@ -137,8 +143,11 @@ const parseCommitMessage = (commitMessage, owner, repo, commit = {}) => {
 
   // if the subject begins with 'word:', treat it as a semantic commit
   if ((match = subject.match(/^(\w+):\s(.*)$/))) {
-    commit.type = match[1].toLocaleLowerCase()
+    const type = match[1].toLocaleLowerCase()
-    subject = match[2]
+    if (knownTypes.has(type)) {
+      commit.type = type
+      subject = match[2]
+    }
   }
 
   // Check for GitHub commit message that indicates a PR
@@ -221,7 +230,6 @@ const parseCommitMessage = (commitMessage, owner, repo, commit = {}) => {
   }
 
   commit.subject = subject.trim()
-
   return commit
 }
 
@@ -389,7 +397,7 @@ const getDependencyCommits = async (pool, from, to) => {
 ****  Main
 ***/
 
-const getNotes = async (fromRef, toRef) => {
+const getNotes = async (fromRef, toRef, newVersion) => {
   if (!fs.existsSync(CACHE_DIR)) {
     fs.mkdirSync(CACHE_DIR)
   }
@@ -437,6 +445,7 @@ const getNotes = async (fromRef, toRef) => {
   // scrape PRs for release note 'Notes:' comments
   for (const commit of pool.commits) {
     let pr = commit.pr
+    let prSubject
     while (pr && !commit.note) {
       const prData = await getPullRequest(pr.number, pr.owner, pr.repo)
       if (!prData || !prData.data) {
@@ -444,30 +453,73 @@ const getNotes = async (fromRef, toRef) => {
       }
 
       // try to pull a release note from the pull comment
-      commit.note = getNoteFromBody(prData.data.body)
+      const prParsed = {}
-      if (commit.note) {
+      parseCommitMessage(`${prData.data.title}\n\n${prData.data.body}`, pr.owner, pr.repo, prParsed)
-        break
+      commit.note = commit.note || prParsed.note
-      }
+      commit.type = commit.type || prParsed.type
+      prSubject = prSubject || prParsed.subject
 
-      // if the PR references another PR, maybe follow it
+      pr = prParsed.pr && (prParsed.pr.number !== pr.number) ? prParsed.pr : null
-      parseCommitMessage(`${prData.data.title}\n\n${prData.data.body}`, pr.owner, pr.repo, commit)
-      pr = pr.number !== commit.pr.number ? commit.pr : null
     }
+
+    // if we still don't have a note, it's because someone missed a 'Notes:
+    // comment in a PR somewhere... use the PR subject as a fallback.
+    commit.note = commit.note || prSubject
   }
 
-  // remove uninteresting commits
+  // remove non-user-facing commits
   pool.commits = pool.commits
     .filter(commit => commit.note !== NO_NOTES)
     .filter(commit => !((commit.note || commit.subject).match(/^[Bb]ump v\d+\.\d+\.\d+/)))
 
+  // if this is a stable release,
+  // remove notes for changes that already landed in a previous major/minor series
+  if (semver.valid(newVersion) && !semver.prerelease(newVersion)) {
+    // load all the prDatas
+    await Promise.all(
+      pool.commits.map(commit => new Promise(async (resolve) => {
+        const { pr } = commit
+        if (typeof pr === 'object') {
+          const prData = await getPullRequest(pr.number, pr.owner, pr.repo)
+          if (prData) {
+            commit.prData = prData
+          }
+        }
+        resolve()
+      }))
+    )
+
+    // remove items that already landed in a previous major/minor series
+    pool.commits = pool.commits
+      .filter(commit => {
+        if (!commit.prData) {
+          return true
+        }
+        const reducer = (accumulator, current) => {
+          if (!semver.valid(accumulator)) { return current }
+          if (!semver.valid(current)) { return accumulator }
+          return semver.lt(accumulator, current) ? accumulator : current
+        }
+        const earliestRelease = commit.prData.data.labels
+          .map(label => label.name.match(/merged\/(\d+)-(\d+)-x/))
+          .filter(label => !!label)
+          .map(label => `${label[1]}.${label[2]}.0`)
+          .reduce(reducer, null)
+        if (!semver.valid(earliestRelease)) {
+          return true
+        }
+        return semver.diff(earliestRelease, newVersion).includes('patch')
+      })
+  }
+
   const notes = {
-    breaks: [],
+    breaking: [],
     docs: [],
     feat: [],
     fix: [],
     other: [],
     unknown: [],
-    ref: toRef
+    name: newVersion
   }
 
   pool.commits.forEach(commit => {
@@ -475,7 +527,7 @@ const getNotes = async (fromRef, toRef) => {
     if (!str) {
       notes.unknown.push(commit)
     } else if (breakTypes.has(str)) {
-      notes.breaks.push(commit)
+      notes.breaking.push(commit)
     } else if (docTypes.has(str)) {
       notes.docs.push(commit)
     } else if (featTypes.has(str)) {
@@ -562,7 +614,7 @@ const renderCommit = (commit, explicitLinks) => {
 }
 
 const renderNotes = (notes, explicitLinks) => {
-  const rendered = [ `# Release Notes for ${notes.ref}\n\n` ]
+  const rendered = [ `# Release Notes for ${notes.name}\n\n` ]
 
   const renderSection = (title, commits) => {
     if (commits.length === 0) {
@@ -582,7 +634,7 @@ const renderNotes = (notes, explicitLinks) => {
     rendered.push(...lines.sort(), '\n')
   }
 
-  renderSection('Breaking Changes', notes.breaks)
+  renderSection('Breaking Changes', notes.breaking)
   renderSection('Features', notes.feat)
   renderSection('Fixes', notes.fix)
   renderSection('Other Changes', notes.other)