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

[MajorLift]

Explanation

• Replace as T[] assertions with ?? []

as assertions intended to exclude an empty/nullable type can be replaced by a nullish coalescing operator converting nullable values into an acceptable empty value that doesn't pollute the variable's type signature.

• Replace as assertions with redundant type guards

Sometimes TypeScript's type narrowing and inference doesn't fully work quite like it should. Even in these cases, using as assertions should be avoided.

It may seem like we know for certain what the type should be and there's no risk of it breaking, but there's always the possibility that some combination of changes elsewhere might affect the typing, in which case as will suppress the alerts TypeScript would otherwise provide us with. This may lead to code breaking silently.

In this example, adding type guards and null checks -- even if they're redundant -- is a small cost to avoid silent failure.

• Make as assertion rely on inference instead of hard-coded type casting

If anything changes in the typing of ChangeCategory or unreleasedChanges that make them inconsistent, this code will fail silently. Relying on type inference instead of explicit type casting will make this logic a little less brittle.

References

• See Add entry forany, as usage in TypeScript Guidelines doc contributor-docs#47

[mcmire]

One suggestion here that could remove all of the typecasts, but it is non-blocking. Thanks for doing this.

[mcmire]

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.

[MajorLift]

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.

[mcmire]

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.

[kanthesha]

how about, adding the tag value part of the error message?

[MajorLift]

You mean something like this?

Suggested change

	`Current version already has tag, which is unexpected for a release candidate.`,
	`Current version already has a tag: '${mostRecentTag}'. This is unexpected for a release candidate.`,

[MajorLift]

@kanthesha Let me know if a26b0bb looks good!

[mcmire]

LGTM!