Skip to content

refactor(semver): removing unreachable code, bringing coverage to 100% #6924

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
kt3k merged 4 commits into from
Jan 7, 2026
+32 −41
Merged

refactor(semver): removing unreachable code, bringing coverage to 100% #6924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e8093fe
Initial commit of coverage work
achedges Dec 29, 2025
7534a30
Removed unreachable code, added tests
achedges Dec 31, 2025
f81d0c8
Inverting comparison for != operator
achedges Dec 31, 2025
55e9326
add fixme comment
kt3k Jan 7, 2026
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ coverage/
.vim
_tmp/
!_tmp/.keep
*.sublime-project
*.sublime-workspace

# tsconfig for bun
/tsconfig.json
2 changes: 1 addition & 1 deletion semver/greater_than_range.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function lessThanComparator(version: SemVer, comparator: Comparator): boolean {
case undefined:
return cmp < 0;
case "!=":
return false;
return cmp >= 0;
Copy link
Contributor Author

@achedges achedges Dec 31, 2025

Choose a reason for hiding this comment

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

I'll admit this one had me scratching my head a bit. It seems that, in the context of this internal utility function, simply inverting the outcome for the != operator (compared to the = operator) should suffice for producing the intended effect.

case ">":
return cmp <= 0;
case "<":
Expand Down
6 changes: 3 additions & 3 deletions semver/greater_than_range_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Deno.test("greaterThanRange() checks if the semver is greater than the range", a
["<1", "1.0.0"],
["=0.7.x", "0.8.2"],
["<0.7.x", "0.7.2"],
["=0.1.0", "1.0.0"],
] as const;

for (const [range, version] of versionGtRange) {
Expand Down Expand Up @@ -178,7 +179,7 @@ Deno.test("greaterThanRange() handles equals operator", () => {
build: [],
};
const range = [[{
operator: "=" as unknown as Operator,
operator: "=" as Operator,
major: 1,
minor: 0,
patch: 0,
Expand All @@ -204,6 +205,5 @@ Deno.test("greaterThanRange() handles not equals operator", () => {
prerelease: [],
build: [],
}]];
// FIXME(kt3k): This demonstrates a bug. This should be false
assertEquals(greaterThanRange(version, range), true);
assertEquals(greaterThanRange(version, range), false);
Copy link
Member

@kt3k kt3k Jan 7, 2026

Choose a reason for hiding this comment

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

oh, thanks for fixing this!

});
2 changes: 1 addition & 1 deletion semver/less_than_range.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function greaterThanComparator(
case undefined:
return cmp > 0;
case "!=":
return false;
return cmp <= 0;
Copy link
Contributor Author

@achedges achedges Dec 31, 2025

Choose a reason for hiding this comment

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

See comments on greater_than_range.ts

case ">":
return false;
case "<":
Expand Down
3 changes: 1 addition & 2 deletions semver/less_than_range_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,5 @@ Deno.test("lessThanRange() handles not equals operator", () => {
prerelease: [],
build: [],
}]];
// FIXME(kt3k): This demonstrates a bug. This should be false
assertEquals(lessThanRange(version, range), true);
assertEquals(lessThanRange(version, range), false);
Copy link
Member

@kt3k kt3k Jan 7, 2026

Choose a reason for hiding this comment

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

👍

});
46 changes: 13 additions & 33 deletions semver/parse_range.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
COMPARATOR_REGEXP,
OPERATOR_XRANGE_REGEXP,
parseBuild,
parseNumber,
parsePrerelease,
XRANGE,
} from "./_shared.ts";
Expand All @@ -27,29 +26,8 @@ function parseComparator(comparator: string): Comparator | null {

if (!groups) return null;

const { operator, prerelease, buildmetadata } =
Copy link
Contributor Author

@achedges achedges Dec 31, 2025

Choose a reason for hiding this comment

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

Unless I'm missing something here, if the regex match succeeds at all, the resulting groups will contain elements for each semver component. This code was attempting to manually parse what this regex was already doing.

groups as ComparatorRegExpGroup;

const semver = groups.major
? {
major: parseNumber(
groups.major,
`Cannot parse comparator ${comparator}: invalid major version`,
),
minor: parseNumber(
groups.minor!,
`Cannot parse comparator ${comparator}: invalid minor version`,
),
patch: parseNumber(
groups.patch!,
`Cannot parse comparator ${comparator}: invalid patch version`,
),
prerelease: prerelease ? parsePrerelease(prerelease) : [],
build: buildmetadata ? parseBuild(buildmetadata) : [],
}
: ANY;

return { operator: operator || undefined, ...semver };
const { operator } = groups as ComparatorRegExpGroup;
return { operator: operator || undefined, ...ANY };
}

function isWildcard(id?: string): boolean {
Expand Down Expand Up @@ -159,7 +137,6 @@ function parseHyphenRange(range: string): Comparator[] | null {
new RegExp(`^${XRANGE}\\s*$`),
);
const rightGroups = rightMatch?.groups;
if (!rightGroups) return null;

const from = handleLeftHyphenRangeGroups(leftGroup as RangeRegExpGroups);
const to = handleRightHyphenRangeGroups(rightGroups as RangeRegExpGroups);
Expand Down Expand Up @@ -347,7 +324,15 @@ function handleEqualOperator(groups: RangeRegExpGroups): Comparator[] {
}
const prerelease = parsePrerelease(groups.prerelease ?? "");
const build = parseBuild(groups.build ?? "");
return [{ operator: undefined, major, minor, patch, prerelease, build }];

Copy link
Contributor Author

@achedges achedges Dec 31, 2025

Choose a reason for hiding this comment

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

We can use the = string if we have it, the net-effect is the same.

return [{
operator: groups.operator === "=" ? "=" : undefined,
major,
minor,
patch,
prerelease,
build,
}];
}

function parseOperatorRange(string: string): Comparator | Comparator[] | null {
Expand All @@ -369,13 +354,8 @@ function parseOperatorRange(string: string): Comparator | Comparator[] | null {
return handleGreaterThanOperator(groups);
case ">=":
return handleGreaterOrEqualOperator(groups);
case "=":
case "":
return handleEqualOperator(groups);
default:
throw new Error(
`Cannot parse version range: '${groups.operator}' is not a valid operator`,
);
return handleEqualOperator(groups);
Copy link
Contributor Author

@achedges achedges Dec 31, 2025

Choose a reason for hiding this comment

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

The existing default case should not be possible, so we can use the equals/empty operator to exercise a switch default.

}
}
function parseOperatorRanges(string: string): (Comparator | null)[] {
Expand Down Expand Up @@ -409,7 +389,7 @@ function parseOperatorRanges(string: string): (Comparator | null)[] {
export function parseRange(value: string): Range {
const result = value
// remove spaces between operators and versions
.replaceAll(/(?<=<|>|=|~|\^)(\s+)/g, "")
.replaceAll(/(?<=[<>=~^])(\s+)/g, "")
.split(/\s*\|\|\s*/)
.map((string) => parseHyphenRange(string) || parseOperatorRanges(string));
if (result.some((r) => r.includes(null))) {
Expand Down
12 changes: 11 additions & 1 deletion semver/parse_range_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ Deno.test("parseRange() parse ranges of different kinds", () => {
["=1.2.3", [
[
{
operator: undefined,
operator: "=",
major: 1,
minor: 2,
patch: 3,
Expand Down Expand Up @@ -665,6 +665,12 @@ Deno.test("parseRange() throws on invalid range", () => {
TypeError,
'Cannot parse version range: range "blerg" is invalid',
);

assertThrows(
() => parseRange("1.b.c"),
TypeError,
'Cannot parse version range: range "1.b.c" is invalid',
);
});

Deno.test("parseRange() handles wildcards", () => {
Expand All @@ -677,6 +683,10 @@ Deno.test("parseRange() handles wildcards", () => {
assertEquals(parseRange("<1.*.*"), [
[{ operator: "<", major: 1, minor: 0, patch: 0 }],
]);
// FIXME(kt3k): The following case should equal `[{ operator: "<", major: 2, minor: 0, patch: 0 }]`
assertEquals(parseRange("<=1.*.2"), [
[{ operator: "<", major: 1, minor: NaN, patch: 0 }],
Copy link
Member

@kt3k kt3k Jan 7, 2026

Choose a reason for hiding this comment

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

I think this should be [{ operator: "<", major: 2, minor: 0, patch: 0 }] (though it's the existing behavior)

npm:semver works like the below:

import * as semver from "npm:semver";

console.log(semver.satisfies("1.9.9", '<=1.*.2')) // => prints true
console.log(semver.satisfies("2.0.0", '<=1.*.2')) // => prints false

I'm adding FIXME comment above for now.

Copy link
Contributor Author

@achedges achedges Jan 7, 2026

Choose a reason for hiding this comment

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

Got it! I'll take a look, thanks for the pointer.

]);
assertEquals(parseRange(">=1.*.0"), [
[{ operator: ">=", major: 1, minor: 0, patch: 0 }],
]);
Expand Down
Loading