-
Notifications
You must be signed in to change notification settings - Fork 643
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
deprecation(semver): deprecate eq()
, neq()
, lt()
, lte()
, gt()
and gte()
#4048
deprecation(semver): deprecate eq()
, neq()
, lt()
, lte()
, gt()
and gte()
#4048
Conversation
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 agree with the overall idea of this PR. I have just a few nits.
It'd be good to hear other's opinions on this change.
// taken over from `eq_test.ts` | ||
Deno.test({ | ||
name: "compare()", | ||
fn: async (t) => { | ||
// [version1, version2] | ||
// version1 should be greater than version2 | ||
const versions: [string, string, number][] = [ | ||
["0.0.0", "0.0.0", 0], | ||
["1.2.3", "1.2.3", 0], | ||
["1.2.3-pre.0", "1.2.3-pre.0", 0], | ||
["1.2.3-pre.0+abc", "1.2.3-pre.0+abc", 0], | ||
["0.0.0", "0.0.0-foo", 1], | ||
["0.0.1", "0.0.0", 1], | ||
["1.0.0", "0.9.9", 1], | ||
["0.10.0", "0.9.0", 1], | ||
["0.99.0", "0.10.0", 1], | ||
["2.0.0", "1.2.3", 1], | ||
["1.2.3", "1.2.3-asdf", 1], | ||
["1.2.3", "1.2.3-4", 1], | ||
["1.2.3", "1.2.3-4-foo", 1], | ||
["1.2.3-5", "1.2.3-5-foo", -1], // numbers > strings, `5-foo` is a string not a number | ||
["1.2.3-5", "1.2.3-4", 1], | ||
["1.2.3-5-foo", "1.2.3-5-Foo", 1], | ||
["3.0.0", "2.7.2+asdf", 1], | ||
["1.2.3-a.10", "1.2.3-a.5", 1], | ||
["1.2.3-a.5", "1.2.3-a.b", -1], | ||
["1.2.3-a.b", "1.2.3-a", 1], | ||
["1.2.3-a.b.c.10.d.5", "1.2.3-a.b.c.5.d.100", 1], | ||
["1.2.3-r2", "1.2.3-r100", 1], | ||
["1.2.3-r100", "1.2.3-R2", 1], | ||
]; | ||
|
||
for (const [v0, v1, expected] of versions) { | ||
await t.step(`${v0} == ${v1}`, () => { | ||
const s0 = parse(v0); | ||
const s1 = parse(v1); | ||
|
||
const eq = compare(s0, s1); | ||
const op = expected ? "==" : "!="; | ||
assertEquals(eq, expected, `${s0} ${op} ${s1}`); | ||
}); | ||
} | ||
}, | ||
}); |
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.
This is the only test needed. It takes care of all of the other equality and inequality cases more precisely.
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
I'd prefer to keep these methods as they are easier to remember than the meaning of return values of |
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Ok, I will close this in favour of another PR. |
ref: #3948 (comment)
eq()
,neq()
,lt()
,lte()
,gt()
andgte()
compare()
jsdoc