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.

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

Test & document numeric flags to fs.open on unix #3641

Closed
wants to merge 3 commits into from

Conversation

XeCycle
Copy link
Contributor

@XeCycle XeCycle commented Nov 3, 2015

For #3628.

Do we prefer two commits here or one? Also, shall we backport the doc commit to older versions?

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 3, 2015
var openPath = path.join(common.tmpDir, 'file-should-not-exist');

// A simple case: O_WRONLY without O_CREAT shall fail with ENOENT
var exceptionCaught = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use assert.throws instead

@@ -340,6 +340,12 @@ The file is created if it does not exist.

* `'ax+'` - Like `'a+'` but fails if `path` exists.

On POSIX platforms `flags` can also be a number as documented by open(2);
Copy link
Member

Choose a reason for hiding this comment

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

Not just POSIX, it works on Windows too. Not all flags from require('constants') are available but the common ones are.

@@ -350,6 +350,10 @@ created. On POSIX systems, `path` is considered to exist even if it is a symlink
to a non-existent file. The exclusive flag may or may not work with network file
systems.

`flags` can also be a number as documented by open(2); commonly used constants

Choose a reason for hiding this comment

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

Not all users know what open(2) means. Would be nice if this was linkified to some docs on it or even better the numbers being documented here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there are many (2) notations in this file; do you want to linkify every one? I suppose that should be another PR anyway.

Choose a reason for hiding this comment

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

Yes, but I can do it in another PR.

@@ -832,6 +836,8 @@ start at 0. The `encoding` can be any one of those accepted by [Buffer][].

If `fd` is specified, `ReadStream` will ignore the `path` argument and will use
the specified file descriptor. This means that no `open` event will be emitted.
Note that `fd` should be blocking; non-blocking `fd`s could be passed to
`net.Socket`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd write this as "should be opened in blocking mode".

Also, s/could/should/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fcntl can set blocking mode after open, so imo "opened in blocking mode" may be inaccurate.

Okay will s/could/should/, not being a native speaker I have to trust you :)

@bnoordhuis
Copy link
Member

Left some comments.

@XeCycle GH doesn't send notifications for new or changed commits, better leave a comment or it won't get noticed until someone goes through the list of open pull requests (like I'm currently doing.)

@XeCycle
Copy link
Contributor Author

XeCycle commented Nov 11, 2015

@bnoordhuis sorry am not aware of that, commenting this time.

Rebased to latest master, and did s/could/should/.

@bnoordhuis
Copy link
Member

LGTM with a question. CI: https://ci.nodejs.org/job/node-test-pull-request/701/

@jasnell
Copy link
Member

jasnell commented Nov 13, 2015

@XeCycle ... this LGTM but can I ask you to please rebase and update?

@XeCycle
Copy link
Contributor Author

XeCycle commented Nov 14, 2015

@jasnell will do that in 48hr. Now out on a vacation.

@XeCycle XeCycle force-pushed the open-num-flag branch 2 times, most recently from 4f6cb6f to 4c90d94 Compare November 16, 2015 01:18
@XeCycle
Copy link
Contributor Author

XeCycle commented Nov 16, 2015

@bnoordhuis @jasnell rebased, and reworded translation on windows to say "where applicable". Please check if these still LGTY.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

LGTM (with a comment)

`flags` can also be a number as documented by open(2); commonly used constants
are available from `require('constants')`. On Windows, flags are translated to
their equivalent ones where acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this last sentence is a rather unclear. I'd recommend including a bit more detail about what is translated to what...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah seems a typo there, I wanted to say "where applicable". Yes, it is unclear; but details imo should go to libuv docs, but no one is taking on that. What about adding a few examples, "e.g. O_EXCL to xxx, O_RDWR to xxx..."?

Copy link
Member

Choose a reason for hiding this comment

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

Examples would be great, I think.

@XeCycle
Copy link
Contributor Author

XeCycle commented Nov 17, 2015

Added a few examples to flags translation.

@bnoordhuis bnoordhuis closed this Nov 22, 2015
@XeCycle XeCycle deleted the open-num-flag branch November 23, 2015 01:39
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
This clarifies that fs.createReadStream and fs.createWriteStream, when
passed a fd, expects the fd to be blocking, and suggests net.Socket as
an alternative.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
This clarifies that fs.createReadStream and fs.createWriteStream, when
passed a fd, expects the fd to be blocking, and suggests net.Socket as
an alternative.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
This clarifies that fs.createReadStream and fs.createWriteStream, when
passed a fd, expects the fd to be blocking, and suggests net.Socket as
an alternative.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
This clarifies that fs.createReadStream and fs.createWriteStream, when
passed a fd, expects the fd to be blocking, and suggests net.Socket as
an alternative.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 17, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 17, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
This clarifies that fs.createReadStream and fs.createWriteStream, when
passed a fd, expects the fd to be blocking, and suggests net.Socket as
an alternative.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants