X Tutup
The Wayback Machine - https://web.archive.org/web/20210814015355/https://github.com/nodejs/node/pull/38569
Skip to content

/ node

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

Jump to bottom

os: add os.devNull #38569

Closed
wants to merge 1 commit into from
Closed

os: add os.devNull #38569

wants to merge 1 commit into from

Conversation

@lpinca
Copy link
Member

@lpinca lpinca commented May 6, 2021 •
edited

Provides the platform-specific file path of the null device.
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 6, 2021

I'm not sure if it makes sense but it is similar to path.sep or path.delimiter. In Python this is available via os.devnull.
@RaisinTen
RaisinTen reviewed May 6, 2021
View changes
doc/api/path.md Outdated

Provides the platform-specific file path of the null device:

* `\\.\nul` on Windows

This comment has been minimized.

Sign in to view
@RaisinTen

RaisinTen May 6, 2021
Member

https://docs.python.org/3/library/os.html#os.devnull says:

'/dev/null' for POSIX, 'nul' for Windows.

So, why isn't this just 'nul' for Windows?

This comment has been minimized.

Sign in to view
@lpinca

lpinca May 6, 2021
Author Member

Because if you do something like

fs.writeFileSync(path.devnull, 'foo');

it does not work as expected.

This comment has been minimized.

Sign in to view
@lpinca

lpinca May 6, 2021
Author Member

See also nodejs/node-v0.x-archive#9271
@RaisinTen
RaisinTen reviewed May 6, 2021
View changes
Copy link
Member

@RaisinTen RaisinTen left a comment

Should we add a test that attempts to write something to the file and then asserts that the file size is 0?
@mscdex
Copy link
Contributor

@mscdex mscdex commented May 6, 2021 •
edited

I'm -0 on adding this, but if anything it should probably live on os and not path since path is about manipulating the paths themselves and retrieving a platform-specific "devnull" path/device is more like a os.tmpdir() thing.
@jasnell
Copy link
Member

@jasnell jasnell commented May 6, 2021

Dropping this off os definitely makes the most sense. I do have questions about the utility of this, however... specifically, what would be the purpose of writing explicitly to nul as opposed to simply just not writing anything?
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 6, 2021 •
edited

@mscdex I thought about that but the same argument can be done for path.sep and path.delimiter. At least there is some consistency. Also the Pyhton implementation lives in the {nt,posix}path module and is then reexported in the os module. It is also available as os.path.devnull.

I don't like to have it on both os and path but we could that if there is consensus.
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 6, 2021 •
edited

@jasnell tests, I used it here https://github.com/lpinca/fritzbox-checksum/blob/fe8311da86062a3c748b9bc3e69abdb28bb9003b/test/test.js#L83.

It could definitely be done differently but why if a null device exists?
@jasnell
Copy link
Member

@jasnell jasnell commented May 6, 2021

Hmmm.. ok. Thinking about it further, hanging it off path would be ok for me so long as it's just a constant similar to separator. If there's even a theoretical need to have to query the os to find the null path, then it should live in os.
@devsnek
Copy link
Member

@devsnek devsnek commented May 6, 2021

+1 to a method in os, even if the implementations just return a static string.
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 6, 2021

@devsnek I'm fine with moving it to os but why a method? There is also os.EOL.
@mscdex
Copy link
Contributor

@mscdex mscdex commented May 6, 2021

I thought about that but the same argument can be done for path.sep and path.delimiter

Those are components of a path, so to me it makes sense for them to live on path.
@lpinca lpinca changed the title path: add path.devnull os: add os.devnull() May 6, 2021
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 6, 2021

Fair enough, moved it to os.devnull().
@lpinca lpinca force-pushed the lpinca:add/path-devnull branch from d35e142 to 7d4759b May 6, 2021
@jasnell
jasnell approved these changes May 6, 2021
View changes
@addaleax addaleax added os and removed path labels May 6, 2021
@lpinca lpinca force-pushed the lpinca:add/path-devnull branch from 7d4759b to 20096b6 May 7, 2021
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@sindresorhus
Copy link

@sindresorhus sindresorhus commented May 7, 2021

I think it should be os.devNull (note the camel casing). It's two words: "dev null". It should also be a property, like os.EOL.
@jasnell
Copy link
Member

@jasnell jasnell commented May 7, 2021

I think it should be os.devNull (note the camel casing). It's two words: "dev null". It should also be a property, like os.EOL.

I agree. I'm ok with the method and the current naming and definitely wouldn't block it (which is why I signed off) but my preference would be a simple property.
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 8, 2021 •
edited

There is os.loadavg(), os.homedir(), os.freemem(), os.totalmem() which all could use camel casing.

os.tmpDir() was explicitly renamed to os.tmpdir() in 3fe6aba. The alias was deprecated in 5e5ec2c and then removed in ab4115f.

os.endianness() also returns just a string constant but it's a method and not a property.

I'm just trying to make things consistent.
@lpinca lpinca force-pushed the lpinca:add/path-devnull branch from 20096b6 to 74d0f9a May 8, 2021
@jasnell
Copy link
Member

@jasnell jasnell commented May 9, 2021

os.endianness() also returns just a string constant but it's a method and not a property.

I always have considered this to be a silly mistake. It never should have been a function and I'd rather not repeat that mistake here. If there's a need to consult the operating system or libuv for the value, that's one thing, but this is just a string constant. I'm -1 on it being a function
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 9, 2021

I always have considered this to be a silly mistake.

The same silly mistake was also done for os.arch() and os.platform() since they do nothing but returning process.arch and process.platform respectively.
@lpinca lpinca force-pushed the lpinca:add/path-devnull branch from 0dc653f to c01e8d2 May 9, 2021
@lpinca lpinca changed the title os: add os.devnull() os: add os.devNull May 9, 2021
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 9, 2021

I've force pushed to move it to os.devNull but in my opinion this creates even more inconsistencies.
@lpinca
os: add os.devNull
1fc60ab 
Provides the platform-specific file path of the null device.
@lpinca lpinca force-pushed the lpinca:add/path-devnull branch from c01e8d2 to 1fc60ab May 9, 2021
@lpinca
Copy link
Member Author

@lpinca lpinca commented May 25, 2021

@Trott @jasnell @RaisinTen @cjihrig is this in its current form acceptable? The check can easily be done in userland so I'm fine with closing if it is controversial.
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@jasnell
Copy link
Member

@jasnell jasnell commented May 25, 2021

Still LGTM!
@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 26, 2021

LGTM
@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented May 26, 2021

LGTM 👍
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@bl-ue
bl-ue approved these changes May 29, 2021
View changes
test/parallel/test-os.js
const devNull = os.devNull;
if (common.isWindows) {
assert.strictEqual(devNull, '\\\\.\\nul');
} else {
assert.strictEqual(devNull, '/dev/null');
}
Comment on lines +263 to +268

This comment has been minimized.

Sign in to view
@bl-ue

bl-ue May 29, 2021 •
edited
Contributor

Any reason not to store os.devNull in a separate variable? I'm not getting it.

Suggested change
const devNull = os.devNull;
if (common.isWindows) {
assert.strictEqual(devNull, '\\\\.\\nul');
} else {
assert.strictEqual(devNull, '/dev/null');
}
if (common.isWindows) {
assert.strictEqual(os.devNull, '\\\\.\\nul');
} else {
assert.strictEqual(os.devNull, '/dev/null');
}

This comment has been minimized.

Sign in to view
@lpinca

lpinca May 29, 2021
Author Member

Consistency with

node/test/parallel/test-os.js

Lines 186 to 191 in 1fc60ab

const EOL = os.EOL;
if (common.isWindows) {
assert.strictEqual(EOL, '\r\n');
} else {
assert.strictEqual(EOL, '\n');
}

@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented May 30, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38375/
@tniessen
tniessen approved these changes May 31, 2021
View changes
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented May 31, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38404/
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Jun 1, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/38430/
@lpinca
Copy link
Member Author

@lpinca lpinca commented Jun 1, 2021

Landed in 152d675
lpinca added a commit that referenced this pull request Jun 1, 2021
@lpinca
os: add os.devNull
152d675 
Provides the platform-specific file path of the null device.

PR-URL: #38569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@lpinca lpinca closed this Jun 1, 2021
@lpinca lpinca deleted the lpinca:add/path-devnull branch Jun 1, 2021
danielleadams added a commit that referenced this pull request Jun 2, 2021
@lpinca @danielleadams
os: add os.devNull
e119531 
Provides the platform-specific file path of the null device.

PR-URL: #38569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danielleadams danielleadams mentioned this pull request Jun 2, 2021
Merged
targos added a commit that referenced this pull request Aug 8, 2021
@lpinca @targos
os: add os.devNull
1733919 
Provides the platform-specific file path of the null device.

PR-URL: #38569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
BethGriggs added a commit that referenced this pull request Aug 12, 2021
@lpinca @BethGriggs
os: add os.devNull
84feb3b 
Provides the platform-specific file path of the null device.

PR-URL: #38569
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell

@Trott Trott

@cjihrig cjihrig

@tniessen tniessen

@RaisinTen RaisinTen

@bl-ue bl-ue

Assignees
No one assigned
Labels
author ready needs-ci os semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

X Tutup