Skip to content

Remove as assertions, or fix to mitigate the risk of silent failure #158

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.

Sign up for GitHub

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

Merged
MajorLift merged 8 commits into from
Oct 17, 2023
Merged

Remove as assertions, or fix to mitigate the risk of silent failure #158

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
53cf673
Replace `as T[]` assertions with `?? []`
MajorLift Oct 13, 2023
f950f3e
Replace `as` assertions with redundant type guards
MajorLift Oct 13, 2023
8da9f21
Make `as` assertion rely on inference instead of hard-coded type casting
MajorLift Oct 13, 2023
033cd4e
Fix type narrowing for `currentVersion` by consolidating conditional …
MajorLift Oct 16, 2023
44cc00d
Install `@metamask/utils` as a devDep
MajorLift Oct 16, 2023
d788964
Replace `Object.keys` with `getKnownPropertyNames` method
MajorLift Oct 16, 2023
a26b0bb
Update src/update-changelog.ts
MajorLift Oct 17, 2023
7b7eef8
Merge branch 'main' into 231013-remove-or-fix-as-assertions
MajorLift Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/changelog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function stringifyRelease(
const categorizedChanges = orderedChangeCategories
.filter((category) => categories[category])
.map((category) => {
const changes = categories[category] as string[];
const changes = categories[category] ?? [];
return stringifyCategory(category, changes);
})
.join('\n\n');
Expand Down Expand Up @@ -383,11 +383,13 @@ export default class Changelog {

const unreleasedChanges = this.#changes[unreleased];

for (const category of Object.keys(unreleasedChanges) as ChangeCategory[]) {
for (const category of Object.keys(
unreleasedChanges,
) as (keyof typeof unreleasedChanges)[]) {
if (releaseChanges[category]) {
releaseChanges[category] = [
...(unreleasedChanges[category] as string[]),
...(releaseChanges[category] as string[]),
...(unreleasedChanges[category] ?? []),
...(releaseChanges[category] ?? []),
];
} else {
releaseChanges[category] = unreleasedChanges[category];
Expand Down
13 changes: 5 additions & 8 deletions src/update-changelog.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ export async function updateChangelog({

if (
isReleaseCandidate &&
mostRecentTag === `${tagPrefixes[0]}${currentVersion || ''}`
mostRecentTag === `${tagPrefixes[0]}${currentVersion ?? ''}`
) {
throw new Error(
`Current version already has tag, which is unexpected for a release candidate.`,
Expand Down Expand Up @@ -250,19 +250,16 @@ export async function updateChangelog({
// Ensure release header exists, if necessary
if (
isReleaseCandidate &&
currentVersion &&
!changelog
.getReleases()
.find((release) => release.version === currentVersion)
) {
// Typecast: currentVersion will be defined here due to type guard at the
// top of this function.
changelog.addRelease({ version: currentVersion as Version });
changelog.addRelease({ version: currentVersion });
Copy link
Contributor

@mcmire mcmire Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is referring to this code at the top of the function:

if (isReleaseCandidate && !currentVersion) {
  throw new Error(
    `A version must be specified if 'isReleaseCandidate' is set.`,
  );
}

Since we're checking for isReleaseCandidate here, there's no way that currentVersion is not set. Hence why I believe that the typecast was added originally. (Why TypeScript doesn't see this, I'm not sure.)

This change doesn't hurt, but I wanted to make sure you saw that.

Copy link
Contributor Author

@MajorLift MajorLift Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consolidated the conditional branches so that we can just rely on type narrowing here: 033cd4e. The tradeoff is that the errors now throw a bit later than they strictly should, but the overhead from that seems minimal.

Copy link
Contributor

@mcmire mcmire Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with these changes we are now always shelling out a couple of times to gather commit hashes and commits regardless of the arguments given. I gather that the current changes were motivated by principle, that generally, if there are invalid inputs, the user wants to know about it right away rather than have to wait a bit. I agree, though, that this doesn't matter, at least right now, so these changes seem fine.

}

if (isReleaseCandidate && hasUnreleasedChanges) {
// Typecast: currentVersion will be defined here due to type guard at the
// top of this function.
changelog.migrateUnreleasedChangesToRelease(currentVersion as Version);
if (isReleaseCandidate && currentVersion && hasUnreleasedChanges) {
changelog.migrateUnreleasedChangesToRelease(currentVersion);
}

const newChangeEntries = newCommits.map(({ prNumber, description }) => {
Expand Down