Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fs: move nullCheck to internal/fs.js to dedupe #8292

Closed
wants to merge 3 commits into from
Closed

fs: move nullCheck to internal/fs.js to dedupe #8292

wants to merge 3 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 27, 2016

Follows up on #8277 by deduping nullCheck() that's used in both fs.js and module.js by introducing internal/fs.js. Blocked by #6413, hence the separate PR. #8277 is intended to be backported to v4.x and v6.x, this PR can live on top of wherever #6413 lives.

See HEAD commit on this branch for what I'm submitting in this PR, the other two belong in #8277.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. build Issues and PRs related to build files or the CI. labels Aug 27, 2016
@rvagg rvagg added the blocked PRs that are blocked by other issues or PRs. label Aug 27, 2016
cjihrig
cjihrig reviewed Aug 27, 2016
View reviewed changes
lib/fs.js
@@ -6,6 +6,7 @@
const constants = process.binding('constants').fs;
const util = require('util');
const pathModule = require('path');
const internalFs = require('internal/fs');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff in this file could be a lot smaller if you add const nullCheck = internalFs.nullCheck; up here.

Copy link
Member Author

@rvagg rvagg Aug 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would, until the next few things are extracted and we get a refactor doing this same thing. Do we have precedent on this with internal? I'm not a fan.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow on the problem. As for a precedent, I'm sure it goes both ways, but as an example -

node/lib/child_process.js

Lines 12 to 17 in a6d53c6

const child_process = require('internal/child_process');
const errnoException = util._errnoException;
const _validateStdio = child_process._validateStdio;
const setupChannel = child_process.setupChannel;
const ChildProcess = exports.ChildProcess = child_process.ChildProcess;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rvagg ... I believe the suggestion is to do:

const internalFs = require('internal/fs');
const nullCheck = internalFs.nullCheck;

Which seems reasonable to me :-)

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2016

The last commit LGTM, with one comment.

@ChALkeR ChALkeR mentioned this pull request Aug 27, 2016
Closed
@bnoordhuis
Copy link
Member

Changes to node_file.cc don't LGTM, see #8277 (comment).

@addaleax
Copy link
Member

I think the first commits here are directly from #8277 and don’t require review (here).

@ChALkeR ChALkeR mentioned this pull request Aug 29, 2016
Closed
2 tasks
@ChALkeR
Copy link
Member

ChALkeR commented Aug 29, 2016

Blocked by #6413,

Good news: not anymore. Not removing the blocked label, though, as this is blocked by #8277 =).

jasnell
jasnell reviewed Sep 4, 2016
View reviewed changes
test/parallel/test-fs-null-bytes.js
@@ -2,56 +2,74 @@
var common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be updated to const since you're in here

@jasnell
Copy link
Member

jasnell commented Sep 4, 2016

@rvagg ... this will need to be rebased now that internal/fs exists already ...

Also, Perhaps the de95b66 commit can be separated from this to a separate PR?

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

ping @rvagg

@rvagg
Copy link
Member Author

rvagg commented Oct 29, 2016

@jasnell db95b66 is in #8277, this builds on top of that, only the last commit here is asking for review. I'm going to hold off on doing anything more with this until #8277 can be resolved but it's blocked because I don't think I'm explaining myself clearly enough or something.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

ping @rvagg

@zimbabao zimbabao mentioned this pull request Jul 3, 2017
Closed
4 tasks
@BridgeAR
Copy link
Member

This seems to have been superseded by ffed7b6. If this actually fixes something else as well, please reopen!

@BridgeAR BridgeAR closed this Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rvagg @cjihrig @bnoordhuis @addaleax @ChALkeR @jasnell @BridgeAR @nodejs-github-bot