Require Node.js 6 and upgrade dependencies #5
Conversation
test.js
Outdated
| @@ -8,5 +8,5 @@ test(t => { | |||
| t.is(m('darwin', '99.0.0'), 'macOS'); | |||
| t.is(m('linux', '3.13.0-24-generic'), 'Linux 3.13'); | |||
| t.is(m('win32', '5.1.2600'), 'Windows XP'); | |||
| t.is(m('win32'), 'Windows'); | |||
| t.is(m('win32'), 'Windows 98'); | |||
There was a problem hiding this comment.
This is not correct. It should continue to be Windows.
There was a problem hiding this comment.
I think the test should be fixed: i first changed the line without even looking at this.
But now that i'm looking i've found this: when i run the tests on linux, the effect is the release is fetched from the current os (in my case Fedora), so the effective call to winRelease in my case is like this
id = winRelease('4.18.13-200.fc28.x86_64');
That is very wrong!
The fix should be on the test, may be passing a valid windows release number? And testing for the real os name, not just the generic "Windows" value as it was.
There was a problem hiding this comment.
If it's not on the current operation system and release is not passed, it should just return the OS name without version number, like before.
|
It should still be possible to run |
|
I was trying to make minimum changes to this package; AFAIK to achieve what you want: Option 1Changes to the That package never returns empty string, and that is the problem for getting just Option 2Avoid calling Make your choice and i can go on with what you think is the best. |
|
👍 Option 2 |
|
Done! Also rebased to simplify history. |
| id = release.replace(/^(\d+\.\d+).*/, '$1'); | ||
| return 'Linux' + (id ? ' ' + id : ''); | ||
| } | ||
|
|
||
| if (platform === 'win32') { | ||
| id = winRelease(release); | ||
| id = release ? winRelease(release) : ''; |
There was a problem hiding this comment.
It should still return Windows 10 if calling osName() or osName('win32') (notice no arguments) on Windows 10.
There was a problem hiding this comment.
So fuzzy... So you want:
When running in the same os than the queried, return full OS + release name
Otherwise, return the OS only
Right?
…ameter, fetch it using os.release()
|
@sindresorhus ping. |
sindresorhus
changed the title
sindresorhus
changed the title
@sindresorhus please review and merge?
The new
windows-releasedetects windows server versions! yeah!