feat: allow checkPlatform to override execution environment #65
Conversation
lib/index.js
Outdated
| if (force) { | ||
| return | ||
| } | ||
|
|
||
| const platform = process.platform | ||
| const arch = process.arch | ||
| const platform = environment && environment.os ? environment.os : process.platform |
There was a problem hiding this comment.
Since we're defaulting environment to {} here we don't need the environment && guard.
There was a problem hiding this comment.
@wraithgar Callers of this function may still pass null or undefined for this argument while the default value is {}. How about leaving the environment && guard as it is and changing the default value to undefined?
There was a problem hiding this comment.
We typically don't guard against bad input. It (arguably unnecessarily) increases the cognitive load of maintaining this module. This is a style choice inside of the npm codebase, we don't need to support the extra falsey value, folks consuming this can omit the parameter.
There was a problem hiding this comment.
@wraithgar Okay, that makes sense! Let's prioritize the consistency across the codebase. 76b0df6
|
This is going to need tests. |
|
I've added tests here: d9b4234 |
yukukotani
changed the title
|
updated the PR title |
|
the PR title linting is broken, had a snafu w/ the template-oss template. Don't worry about it. |
Co-authored-by: Gar <wraithgar@github.com>
|
@wraithgar Hi, sorry for bothering you but let me kindly remind. When is this getting released and included in npm/cli? I'd like to start working on npm/rfcs#612. |
|
@yukukotani This will be released with the next |
This PR allows
checkPlatformto override execution environment which comes fromprocess.platformandprocess.archby default.This will be required to achieve npm/rfcs#612
References