-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
fs: replace magic numbers by named constants #18757
Conversation
lib/fs.js
Outdated
if (ch === 92/*'\'*/ || ch === 47/*'/'*/) | ||
const isSeparator = ch === CHAR_BACKWARD_SLASH || | ||
ch === CHAR_FORWARD_SLASH; | ||
if (isSeparator) |
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.
Nit: the extra variable could also just be replaces with a comment.
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.
@BridgeAR done
de630e8
to
a986405
Compare
lib/fs.js
Outdated
@@ -1838,7 +1842,9 @@ if (isWindows) { | |||
nextPart = function nextPart(p, i) { | |||
for (; i < p.length; ++i) { | |||
const ch = p.charCodeAt(i); | |||
if (ch === 92/*'\'*/ || ch === 47/*'/'*/) | |||
|
|||
// if separator |
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.
Nit: could this be changed so something like "Check for a separator character"?
And please always begin comments with a capital letter :-)
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.
@BridgeAR ok, sorry
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.
@BridgeAR done
a986405
to
28ac762
Compare
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.
sure
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 if CI is green
CI before landing: https://ci.nodejs.org/job/node-test-pull-request/13184/ |
New CI since we had infra problems on OSX yesterday: https://ci.nodejs.org/job/node-test-pull-request/13199/ |
PR-URL: #18757 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Landed in d8d84ee. Thanks! |
PR-URL: #18757 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
PR-URL: #18757 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
PR-URL: nodejs#18757 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
fs