Implement .rcompare() method
#30
Conversation
src/index.ts
Outdated
| * @returns {number} Returns `0` if `versionA == versionB`, | ||
| * or `-1` if `versionA` is greater, or `1` if `versionB` is greater. | ||
| * null values are always weighted above string values, and string values are always | ||
| * weighted above valid semver values. |
There was a problem hiding this comment.
You've flipped the order of this description (B > A, instead of A > B), and also the ordering of these comparisons (null > string, instead of string > null). I think that makes this 2nd sentence here wrong, as it's kind of become a double negative.
In practice, rcompare(null, 'a string') and rcompare('a string', '1.2.3') return 1, and this description says the opposite.
There was a problem hiding this comment.
I'll tidy up the description here and on the compare method
src/index.ts
Outdated
| * | ||
| * resinSemver.compare('Resin OS 2.0.0+rev4 (prod)', 'Resin OS 1.16.0'); //--> 1 | ||
| * | ||
| * resinSemver.compare('Resin OS 1.16.0', 'Resin OS 1.16.0'); //--> 0 |
There was a problem hiding this comment.
These examples should all be rcompare, not compare.
src/index.ts
Outdated
| * | ||
| * resinSemver.compare('Resin OS 1.16.0', 'Resin OS 2.0.0+rev4 (prod)'); //--> -1 | ||
| * | ||
| * resinSemver.compare('Resin OS 2.0.0+rev4 (prod)', 'Resin OS 1.16.0'); //--> 1 |
There was a problem hiding this comment.
You've flipped the result of the three examples above correctly, but they've got the wrong results, I think? Looks like that's because they're wrong in compare too.
I dream of a world in which python's doctest is standard in every other language, but until then, we're going to have to endless manually sanity check example docs like this 😭. If you've got a brilliant idea for including the tests as examples in here though, I'd love to hear it.
There was a problem hiding this comment.
@pimterry I did some experimentation and I'm able to programmatically get output like this https://github.com/resin-io-modules/resin-semver/blob/docs-test/test.md
Ideally, I'd like to prettify the output more, but I think this is a good start. What do you think?
There was a problem hiding this comment.
Haha, that is awesome. Yes, output like that would be amazing, and lets us skip this manual example writing entirely.
This is extremely cool. I can't overstate how cool this is. I'd love it for the SDK, and I'm sure we could come up with all sorts of other places in future too across the whole of Resin, we've got loads of docs like this that are messy and easily get inaccurate. Good encouragement for us to get useful tests in place too. Even outside Resin, I expect we could make an exciting visible open-source project from it that other people would want to use. I'm super into this.
Not sure if it's useful, but one thing that might help make the code for this a little simpler would be to hook it into the actual testing process, with a mocha reporter, rather than having to run TS and find the test file(s) yourself. There's already a built-in mocha reporter that outputs markdown with the results and body of all the tests run, which might help here: https://github.com/mochajs/mocha/blob/master/lib/reporters/markdown.js#L86-L92.
There was a problem hiding this comment.
oooh yep the markdown reporter is a revelation!
There was a problem hiding this comment.
There was a problem hiding this comment.
I've merged the auto generated example into this branch and I'll look into separating the code into a standalone module in the future
src/index.ts
Outdated
| * resinSemver.compare('Resin OS 1.16.0', 'Resin OS 1.16.0'); //--> 0 | ||
| */ | ||
| export const rcompare = (versionA: string | null, versionB: string | null): number => { | ||
| return 0 - compare(versionA, versionB); |
There was a problem hiding this comment.
I don't want to sound silly, but why aren't we using the unary - operator?
There was a problem hiding this comment.
I assumed it's just a clarity/style thing - I'm fine with either one, whatever everybody else prefers
Connects to #24 JSDoc examples are now automatically generated using the mocha markdown reporter change-type: minor
LucianBuzzo
force-pushed
the
24-rcompare
branch
from
37a8cbc
to
1f1cf85
Compare
Connects to #24
change-type: minor