-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: do not run IsWindowsBatchFile on non-windows #55560
src: do not run IsWindowsBatchFile on non-windows #55560
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55560 +/- ##
=======================================
Coverage 88.42% 88.43%
=======================================
Files 654 654
Lines 187662 187656 -6
Branches 36121 36121
=======================================
+ Hits 165939 165949 +10
+ Misses 14962 14951 -11
+ Partials 6761 6756 -5
|
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.
LGTM. I'd include the comment inside the #ifdef but that's my own preference and not a blocker.
I specifically wrote it that way so that IsWindowsBatchFile and its call sites get type-checked by the compiler always and everywhere. Of course the function was still small and nimble back then. |
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.
LGTM
Landed in e160e54 |
PR-URL: nodejs#55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
PR-URL: #55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
PR-URL: nodejs#55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
PR-URL: nodejs#55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
PR-URL: #55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
PR-URL: #55560 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]>
Let's not run
IsWindowsBatchFile
on non-windows environments.