src: prefer param function check over args length #23835

codebytere · 2018-10-23T15:51:49Z

We have to override some fs methods in Electron, and as I looked at their original impls I believe that checking the type of the second argument as opposed to simply the number of arguments passed is more consistent and reliable.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-10-23T15:51:51Z

@codebytere build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1346/pipeline

lib/fs.js

refack · 2018-10-23T16:39:10Z

👍

I had a thought once about doing something like:

function fstat(fd, ...args) {
  const lastArg = args.pop();
  const callback = (typeof lastArg === 'function') ? lastArg : throw ..;
  const options = args.pop() || {};
  ...
}

Actually what we need is a generic arg validator :dream:

function fstat(...args) {
  const { fd, options, callback } = validateArgs(args, { some sort of contract });

mscdex · 2018-10-23T20:50:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18086/

mhdawson

LGTM

lpinca · 2018-10-24T19:43:13Z

lib/fs.js

@@ -412,7 +412,7 @@ function open(path, flags, mode, callback) {
  path = toPathIfFileURL(path);
  validatePath(path);
  const flagsNumber = stringToFlags(flags);
-  if (arguments.length < 4) {
+  if (typeof mode === 'function') {


I think this will conflict with @bnoordhuis's work on #23767

Should we land #23767 first and then adapt this PR to that change? Seems do-able. /cc @codebytere @bnoordhuis

Trott · 2018-11-06T05:20:28Z

@codebytere This seems ready to go if we can get a rebase to resolve the merge conflict with #23767...

codebytere · 2018-11-06T05:22:25Z

@Trott i'll handle first thing tomorrow!

codebytere · 2018-11-06T19:21:17Z

@Trott should be set! i couldn't change this line becuase if i simply checked for flags being a function that didn't properly account for the error case where no callback was passed (i.e. only one arg passed) and would improperly fail tests with ERR_INVALID_OPT_VALUE instead of ERR_INVALID_CALLBACK.

refack · 2018-11-06T21:13:48Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18378/

Trott · 2018-11-08T03:42:13Z

Landed in 7cf5679

PR-URL: nodejs#23835 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

PR-URL: #23835 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

PR-URL: nodejs#23835 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

PR-URL: #23835 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 23, 2018

cjihrig reviewed Oct 23, 2018

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

refack approved these changes Oct 23, 2018

View reviewed changes

mmarchini approved these changes Oct 23, 2018

View reviewed changes

cjihrig approved these changes Oct 23, 2018

View reviewed changes

addaleax approved these changes Oct 23, 2018

View reviewed changes

mhdawson approved these changes Oct 23, 2018

View reviewed changes

jasnell approved these changes Oct 23, 2018

View reviewed changes

jdalton approved these changes Oct 24, 2018

View reviewed changes

hiroppy approved these changes Oct 24, 2018

View reviewed changes

lpinca reviewed Oct 24, 2018

View reviewed changes

src: prefer param function check over args length

4f86c6e

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2018

Trott closed this Nov 8, 2018

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

This was referenced Nov 15, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#232

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-android-nodejs#249

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#233

Closed

This was referenced Nov 16, 2018

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#234

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#235

Closed

2018-11-15 Version 11.2.0 (Current) @BridgeAR beevelop/docker-nodejs#236

Closed

codebytere added the land-on-v10.x label Nov 29, 2018

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: prefer param function check over args length #23835

src: prefer param function check over args length #23835

codebytere commented Oct 23, 2018 •

edited

Loading

nodejs-github-bot commented Oct 23, 2018

refack commented Oct 23, 2018

mscdex commented Oct 23, 2018

mhdawson left a comment

lpinca Oct 24, 2018

Trott Oct 25, 2018

Trott commented Nov 6, 2018

codebytere commented Nov 6, 2018

codebytere commented Nov 6, 2018 •

edited

Loading

refack commented Nov 6, 2018

Trott commented Nov 8, 2018

src: prefer param function check over args length #23835

src: prefer param function check over args length #23835

Conversation

codebytere commented Oct 23, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 23, 2018

refack commented Oct 23, 2018

mscdex commented Oct 23, 2018

mhdawson left a comment

Choose a reason for hiding this comment

lpinca Oct 24, 2018

Choose a reason for hiding this comment

Trott Oct 25, 2018

Choose a reason for hiding this comment

Trott commented Nov 6, 2018

codebytere commented Nov 6, 2018

codebytere commented Nov 6, 2018 • edited Loading

refack commented Nov 6, 2018

Trott commented Nov 8, 2018

codebytere commented Oct 23, 2018 •

edited

Loading

codebytere commented Nov 6, 2018 •

edited

Loading