fs: improve error message for invalid flag #5590

jasnell · 2016-03-07T20:24:13Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

fs

Description of change

Flags on fs.open and others can be passed as strings or int.
Previously, if passing anything other than string or int,
the error message would only say that flags must be an int.

jasnell · 2016-03-07T20:25:36Z

This one isn't obvious... basically, by changing the early out condition on stringToFlag it falls through to the error that is reported if the string flag is not correct.

cjihrig · 2016-03-07T20:27:39Z

Could you add a test?

jasnell · 2016-03-07T20:33:30Z

@cjihrig ... done!

jasnell · 2016-03-07T20:35:07Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/1873/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/1874/

cjihrig · 2016-03-07T20:41:28Z

LGTM if the CI is happy

jasnell · 2016-03-07T21:41:12Z

Generally CI is good but there was a build bot error on OSX... trying again: https://ci.nodejs.org/job/node-test-pull-request/1877/

jasnell · 2016-03-08T00:02:30Z

Ok, looks like something is borked with the OSX build bot (cc @nodejs/build). Otherwise this looks ok. Tests pass locally on OSX. Given that this is a major, I'll give it until tomorrow or Wednesday to land.

jasnell · 2016-03-09T04:03:41Z

Fixes: #2871

MylesBorins · 2016-03-09T04:29:50Z

LGTM

bnoordhuis · 2016-03-09T09:37:14Z

Sign in to view

+
+assert.throws(
+  () => fs._stringToFlags({}),
+  (err) => /^Unknown file open flag: \[object Object\]/.test(err.message)


You can shorten this to just the regex, i.e.:

assert.throws( () => fs._stringToFlags({}), /^Unknown file open flag: \[object Object\]/);

bnoordhuis · 2016-03-09T09:37:22Z

LGTM with a suggestion.

Flags on fs.open and others can be passed as strings or int. Previously, if passing anything other than string or int, the error message would only say that flags must be an int.

Flags on fs.open and others can be passed as strings or int. Previously, if passing anything other than string or int, the error message would only say that flags must be an int. PR-URL: #5590 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PR-URL: #5590 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2016-03-09T20:35:05Z

Landed in 8bb60e3 and a5aa7c1

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

jasnell added fs semver-major error-handling labels Mar 7, 2016

rvagg added the dont-land-on-v4.x label Mar 7, 2016

jasnell mentioned this pull request Mar 9, 2016

fs: fix confusing flags TypeError msg #2902

Closed

bnoordhuis reviewed Mar 9, 2016
View changes

jasnell added 2 commits Mar 7, 2016

fs: improve error message for invalid flag

a255da4

Flags on fs.open and others can be passed as strings or int. Previously, if passing anything other than string or int, the error message would only say that flags must be an int.

test: expand test case for unknown file open flags

45ee049

jasnell force-pushed the jasnell:flags-fix branch to 45ee049 Mar 9, 2016

jasnell added a commit that referenced this pull request Mar 9, 2016

test: expand test case for unknown file open flags

a5aa7c1

PR-URL: #5590 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell closed this Mar 9, 2016

jasnell mentioned this pull request Mar 9, 2016

fs: bad error message in fs.openSync #2871

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

nodejs / node

fs: improve error message for invalid flag #5590

fs: improve error message for invalid flag #5590

jasnell commented Mar 7, 2016

jasnell commented Mar 7, 2016

cjihrig commented Mar 7, 2016

jasnell commented Mar 7, 2016

jasnell commented Mar 7, 2016

cjihrig commented Mar 7, 2016

jasnell commented Mar 7, 2016

jasnell commented Mar 8, 2016

jasnell commented Mar 9, 2016

MylesBorins commented Mar 9, 2016

This comment has been minimized.

bnoordhuis commented Mar 9, 2016

jasnell commented Mar 9, 2016

nodejs / node

fs: improve error message for invalid flag #5590

fs: improve error message for invalid flag #5590

Conversation

jasnell commented Mar 7, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

jasnell commented Mar 7, 2016

cjihrig commented Mar 7, 2016

jasnell commented Mar 7, 2016

jasnell commented Mar 7, 2016

cjihrig commented Mar 7, 2016

jasnell commented Mar 7, 2016

jasnell commented Mar 8, 2016

jasnell commented Mar 9, 2016

MylesBorins commented Mar 9, 2016

This comment has been minimized.

bnoordhuis Mar 9, 2016 Member

bnoordhuis commented Mar 9, 2016

jasnell commented Mar 9, 2016

bnoordhuis Mar 9, 2016
Member