refactor(semver): removing unreachable code, bringing coverage to 100% #6924
Conversation
| return cmp < 0; | ||
| case "!=": | ||
| return false; | ||
| return cmp >= 0; |
There was a problem hiding this comment.
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.
| return cmp > 0; | ||
| case "!=": | ||
| return false; | ||
| return cmp <= 0; |
There was a problem hiding this comment.
See comments on greater_than_range.ts
|
|
||
| if (!groups) return null; | ||
|
|
||
| const { operator, prerelease, buildmetadata } = |
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6924 +/- ##
==========================================
+ Coverage 94.17% 94.27% +0.09%
==========================================
Files 584 584
Lines 43087 43131 +44
Branches 6875 6903 +28
==========================================
+ Hits 40578 40660 +82
+ Misses 2457 2420 -37
+ Partials 52 51 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const prerelease = parsePrerelease(groups.prerelease ?? ""); | ||
| const build = parseBuild(groups.build ?? ""); | ||
| return [{ operator: undefined, major, minor, patch, prerelease, build }]; | ||
|
|
There was a problem hiding this comment.
We can use the = string if we have it, the net-effect is the same.
| throw new Error( | ||
| `Cannot parse version range: '${groups.operator}' is not a valid operator`, | ||
| ); | ||
| return handleEqualOperator(groups); |
There was a problem hiding this comment.
The existing default case should not be possible, so we can use the equals/empty operator to exercise a switch default.
| }]]; | ||
| // FIXME(kt3k): This demonstrates a bug. This should be false | ||
| assertEquals(greaterThanRange(version, range), true); | ||
| assertEquals(greaterThanRange(version, range), false); |
| }]]; | ||
| // FIXME(kt3k): This demonstrates a bug. This should be false | ||
| assertEquals(lessThanRange(version, range), true); | ||
| assertEquals(lessThanRange(version, range), false); |
| [{ operator: "<", major: 1, minor: 0, patch: 0 }], | ||
| ]); | ||
| assertEquals(parseRange("<=1.*.2"), [ | ||
| [{ operator: "<", major: 1, minor: NaN, patch: 0 }], |
There was a problem hiding this comment.
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 falseI'm adding FIXME comment above for now.
There was a problem hiding this comment.
Got it! I'll take a look, thanks for the pointer.
kt3k
left a comment
kt3k
left a comment
There was a problem hiding this comment.
Thank you for working on this! LGTM
2 participants
This change refactors a handful of internal functionality to remove unreachable code, and adds a couple of new tests to bring up line coverage.
Comments added inline, I hope this helps!