-
-
Notifications
You must be signed in to change notification settings - Fork 4
Fix bug introduced to updateChangelog in #158
#181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3008846
8cec96e
d18d0fa
aeabb00
da39a58
3aef0c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { strict as assert } from 'assert'; | |
| import execa from 'execa'; | ||
|
|
||
| import type Changelog from './changelog'; | ||
| import { Formatter } from './changelog'; | ||
| import { Formatter, getKnownPropertyNames } from './changelog'; | ||
| import { ChangeCategory, Version } from './constants'; | ||
| import { parseChangelog } from './parse-changelog'; | ||
| import { PackageRename } from './shared-types'; | ||
|
|
@@ -20,6 +20,9 @@ async function getMostRecentTag({ | |
| }: { | ||
| tagPrefixes: [string, ...string[]]; | ||
| }) { | ||
| // Ensure we have all tags on remote | ||
| await runCommand('git', ['fetch', '--tags']); | ||
|
|
||
| let mostRecentTagCommitHash: string | null = null; | ||
| for (const tagPrefix of tagPrefixes) { | ||
| const revListArgs = [ | ||
|
|
@@ -157,6 +160,53 @@ async function getCommitHashesInRange( | |
| return await runCommand('git', revListArgs); | ||
| } | ||
|
|
||
| type AddNewCommitsOptions = { | ||
| mostRecentTag: string | null; | ||
| repoUrl: string; | ||
| loggedPrNumbers: string[]; | ||
| projectRootDirectory?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Get the list of new change entries to add to a changelog. | ||
| * | ||
| * @param options - Options. | ||
| * @param options.mostRecentTag - The most recent tag. | ||
| * @param options.repoUrl - The GitHub repository URL for the current project. | ||
| * @param options.loggedPrNumbers - A list of all pull request numbers included in the relevant parsed changelog. | ||
| * @param options.projectRootDirectory - The root project directory, used to | ||
| * filter results from various git commands. This path is assumed to be either | ||
| * absolute, or relative to the current directory. Defaults to the root of the | ||
| * current git repository. | ||
| * @returns A list of new change entries to add to the changelog, based on commits made since the last release. | ||
| */ | ||
| async function getNewChangeEntries({ | ||
| mostRecentTag, | ||
| repoUrl, | ||
| loggedPrNumbers, | ||
| projectRootDirectory, | ||
| }: AddNewCommitsOptions) { | ||
| const commitRange = | ||
| mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`; | ||
| const commitsHashesSinceLastRelease = await getCommitHashesInRange( | ||
| commitRange, | ||
| projectRootDirectory, | ||
| ); | ||
| const commits = await getCommits(commitsHashesSinceLastRelease); | ||
|
|
||
| const newCommits = commits.filter( | ||
| ({ prNumber }) => !prNumber || !loggedPrNumbers.includes(prNumber), | ||
| ); | ||
|
|
||
| return newCommits.map(({ prNumber, description }) => { | ||
| if (prNumber) { | ||
| const suffix = `([#${prNumber}](${repoUrl}/pull/${prNumber}))`; | ||
| return `${description} ${suffix}`; | ||
| } | ||
| return description; | ||
| }); | ||
| } | ||
|
|
||
| export type UpdateChangelogOptions = { | ||
| changelogContent: string; | ||
| currentVersion?: Version; | ||
|
|
@@ -213,43 +263,17 @@ export async function updateChangelog({ | |
| packageRename, | ||
| }); | ||
|
|
||
| // Ensure we have all tags on remote | ||
| await runCommand('git', ['fetch', '--tags']); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is moved to |
||
| const mostRecentTag = await getMostRecentTag({ | ||
| tagPrefixes, | ||
| }); | ||
|
|
||
| const commitRange = | ||
| mostRecentTag === null ? 'HEAD' : `${mostRecentTag}..HEAD`; | ||
| const commitsHashesSinceLastRelease = await getCommitHashesInRange( | ||
| commitRange, | ||
| projectRootDirectory, | ||
| ); | ||
| const commits = await getCommits(commitsHashesSinceLastRelease); | ||
|
|
||
| const loggedPrNumbers = getAllLoggedPrNumbers(changelog); | ||
| const newCommits = commits.filter( | ||
| ({ prNumber }) => | ||
| prNumber === undefined || !loggedPrNumbers.includes(prNumber), | ||
| ); | ||
|
Comment on lines
-222
to
-234
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section is moved to |
||
|
|
||
| const hasUnreleasedChanges = | ||
| Object.keys(changelog.getUnreleasedChanges()).length !== 0; | ||
| if ( | ||
| newCommits.length === 0 && | ||
| (!isReleaseCandidate || hasUnreleasedChanges) | ||
| ) { | ||
| return undefined; | ||
| } | ||
|
Comment on lines
-238
to
-243
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, the |
||
|
|
||
| if (isReleaseCandidate) { | ||
| if (!currentVersion) { | ||
| throw new Error( | ||
| `A version must be specified if 'isReleaseCandidate' is set.`, | ||
| ); | ||
| } | ||
|
|
||
| if (mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`) { | ||
| if (mostRecentTag === `${tagPrefixes[0]}${currentVersion}`) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I just realized that we only use the first tag prefix here, which seems strange. This may be a potential bug. But we can investigate that later.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The jsdoc does specify that the first prefix is the intended prefix and the rest are fallbacks.
But this also wasn't a change introduced in this PR, so it should get its own ticket. |
||
| throw new Error( | ||
| `Current version already has a tag ('${mostRecentTag}'), which is unexpected for a release candidate.`, | ||
| ); | ||
|
|
@@ -264,28 +288,31 @@ export async function updateChangelog({ | |
| changelog.addRelease({ version: currentVersion }); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| if (hasUnreleasedChanges) { | ||
| const hasUnreleasedChangesToRelease = | ||
| getKnownPropertyNames(changelog.getUnreleasedChanges()).length > 0; | ||
| if (hasUnreleasedChangesToRelease) { | ||
| changelog.migrateUnreleasedChangesToRelease(currentVersion); | ||
| } | ||
| } | ||
|
|
||
| const newChangeEntries = newCommits.map(({ prNumber, description }) => { | ||
| if (prNumber) { | ||
| const suffix = `([#${prNumber}](${repoUrl}/pull/${prNumber}))`; | ||
| return `${description} ${suffix}`; | ||
| } | ||
| return description; | ||
| }); | ||
|
Comment on lines
-271
to
-277
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section is moved to |
||
| const newChangeEntries = await getNewChangeEntries({ | ||
| mostRecentTag, | ||
| repoUrl, | ||
| loggedPrNumbers: getAllLoggedPrNumbers(changelog), | ||
| projectRootDirectory, | ||
| }); | ||
|
|
||
| for (const description of newChangeEntries.reverse()) { | ||
| changelog.addChange({ | ||
| version: isReleaseCandidate ? currentVersion : undefined, | ||
| category: ChangeCategory.Uncategorized, | ||
| description, | ||
| }); | ||
| } | ||
| for (const description of newChangeEntries.reverse()) { | ||
| changelog.addChange({ | ||
| version: isReleaseCandidate ? currentVersion : undefined, | ||
| category: ChangeCategory.Uncategorized, | ||
| description, | ||
| }); | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the bracket that caused the bug. Moving it to above the |
||
|
|
||
| return changelog.toString(); | ||
| const newChangelogContent = changelog.toString(); | ||
| const isChangelogUpdated = changelogContent !== newChangelogContent; | ||
| return isChangelogUpdated ? newChangelogContent : undefined; | ||
|
Comment on lines
+313
to
+315
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a possibility to be little more flexible here!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing these out! Unfortunately, this one results in a linter error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would work, but I would prefer to prioritize type safety over brevity here, since
mostRecentTagis derived from the output of running an actual git command in shell. There are no strong guarantees on what the runtime output will be, so we can't completely rule out edge cases where e.g. an empty string or undefined is returned.This probably aligns with the original motivation for
getMostRecentTagreturningnullin its early exit branch. It represents the exception case where we know for sure that no tag exists, which is distinct from the error case where some tags exist, but the most recent tag could not be found due to a failure or edge case. This second case would be better represented by anundefined.What are your thoughts? In general, I feel less comfortable replacing
=== nullwith!, because our codebase mostly usesundefined, which makes me assume that anynullusage is intentional.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we did use
nullintentionally to represent lack of a most recent tag, so the use ofnullmatches the type thatgetMostRecentTagreturns. However, I suppose that the tag name could be an empty string in theory if for some reasongit describereturns an empty string, and that case we would want to fall back toHEAD. So, we could updategetMostRecentTagto just returnstringinstead ofstring | null, except that in case there is no recent tag, it would thenreturn '';. That might look strange, but we could add a comment saying why we're doing that or give that empty string a name that describe its purpose.In any case, it seems that this line was merely copied from another function, so maybe we want to make that change in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like using
nullfor clear distinction and visibility, but in any case I agree with revisiting this in its own ticket since this wasn't a change introduced by this PR.