fs: improve mode and flags validation #27044
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
the
fs
|
This mainly fixes the bug but I mark this defensively as semver-major. CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1791/ |
BridgeAR
added
the
semver-major
|
@nodejs/fs @nodejs/tsc PTAL |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
approved these changes
Apr 3, 2019
BridgeAR
added
the
author ready
This comment has been minimized.
This comment has been minimized.
|
@nodejs/fs @nodejs/tsc PTAL |
This comment has been minimized.
This comment has been minimized.
Trott
approved these changes
Apr 6, 2019
|
This already has some LGs but it needs one more LG from the @nodejs/tsc. PTAL |
mcollina
reviewed
Apr 8, 2019
BridgeAR
added
wip
BridgeAR
force-pushed
the
fix-fs-validation
branch
from
78fbc19
to
9bf11e8
Compare
BridgeAR
force-pushed
the
fix-fs-validation
branch
from
9bf11e8
to
0d59092
Compare
|
I finally got the time to get back to this. The last time I missed a few more bugs in the beginning. That was very subtle because the name of the arguments was deceiving me. I fixed the variable naming and added proper validation for PTAL |
targos
approved these changes
Sep 26, 2019
|
I just pushed two more commits that add some more refinements to the docs and fix all left issues with |
This comment has been minimized.
This comment has been minimized.
targos
approved these changes
Sep 26, 2019
BridgeAR
force-pushed
the
fix-fs-validation
branch
from
6de850d
to
05b29f3
Compare
|
@jasnell @targos @Trott @bnoordhuis please confirm your LG so that this can land. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
BridgeAR
force-pushed
the
fix-fs-validation
branch
from
05b29f3
to
f1cc9cb
Compare
This comment has been minimized.
This comment has been minimized.
Multiple `fs` functions have a `mode` and or `flag` option. In some cases those have been mistaken and named wrongly. All of those faulty entries have been fixed. Besides that some indentation is also aligned with the rest of the document.
This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted strings as mode. It should have only accepted numbers. It will now always validate the flags and the mode argument in an consistent way.
This consolidates some examples to concentrate the reader on the important aspects and to reduce reading overhead.
BridgeAR
force-pushed
the
fix-fs-validation
branch
from
f1cc9cb
to
acc80ae
Compare
|
@jasnell @targos @Trott @bnoordhuis please confirm your LG so that this can land. |
Trott
approved these changes
Jan 12, 2020
BridgeAR
added
author ready
BridgeAR
added a commit
that referenced
this pull request
Jan 12, 2020
Multiple `fs` functions have a `mode` and or `flag` option. In some cases those have been mistaken and named wrongly. All of those faulty entries have been fixed. Besides that some indentation is also aligned with the rest of the document. PR-URL: #27044 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR
added a commit
that referenced
this pull request
Jan 12, 2020
This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted strings as mode. It should have only accepted numbers. It will now always validate the flags and the mode argument in an consistent way. PR-URL: #27044 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BridgeAR
added a commit
that referenced
this pull request
Jan 12, 2020
This consolidates some examples to concentrate the reader on the important aspects and to reduce reading overhead. PR-URL: #27044 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
Landed in af5ddf2...4455f60 🎉 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug in
fs.promises.accessas it accepted strings asmode. It should have only accepted numbers. It will now always
validate the flags and the mode argument in an consistent way.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes