FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Test & document numeric flags to fs.open on unix by XeCycle · Pull Request #3641 · nodejs/node · GitHub

/ node Public

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

Closed
XeCycle wants to merge 3 commits into
nodejs:masterfrom
XeCycle:open-num-flag
ConversationCommits (3)ChecksFiles changed
Closed

Test & document numeric flags to fs.open on unix#3641
XeCycle wants to merge 3 commits into
nodejs:masterfrom
XeCycle:open-num-flag

Conversation

XeCycle commented Nov 3, 2015

Copy link
Copy Markdown
Contributor

For #3628.

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

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 3, 2015
thefourtheye reviewed Nov 3, 2015
View reviewed changes
Comment thread test/parallel/test-fs-open-numeric-flags-posix.js Outdated

thefourtheye Nov 3, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Use assert.throws instead

bnoordhuis reviewed Nov 3, 2015
View reviewed changes
Comment thread doc/api/fs.markdown Outdated

bnoordhuis Nov 3, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

sindresorhus reviewed Nov 4, 2015
View reviewed changes
Comment thread doc/api/fs.markdown

sindresorhus Nov 4, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

XeCycle Nov 4, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

sindresorhus Nov 4, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, but I can do it in another PR.

sindresorhus mentioned this pull request Nov 4, 2015
Closed
bnoordhuis reviewed Nov 10, 2015
View reviewed changes
Comment thread doc/api/fs.markdown Outdated

bnoordhuis Nov 10, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Also, s/could/should/

XeCycle Nov 11, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Nov 10, 2015

Copy link
Copy Markdown
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 commented Nov 11, 2015

Copy link
Copy Markdown
Contributor Author

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

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

bnoordhuis commented Nov 11, 2015

Copy link
Copy Markdown
Member

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

jasnell commented Nov 13, 2015

Copy link
Copy Markdown
Member

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

XeCycle commented Nov 14, 2015

Copy link
Copy Markdown
Contributor Author

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

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

XeCycle commented Nov 16, 2015

Copy link
Copy Markdown
Contributor Author

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

jasnell commented Nov 16, 2015

Copy link
Copy Markdown
Member

LGTM (with a comment)

jasnell reviewed Nov 16, 2015
View reviewed changes
Comment thread doc/api/fs.markdown Outdated

jasnell Nov 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

XeCycle Nov 16, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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..."?

jasnell Nov 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Examples would be great, I think.

XeCycle commented Nov 17, 2015

Copy link
Copy Markdown
Contributor Author

Added a few examples to flags translation.

bnoordhuis closed this Nov 22, 2015
XeCycle deleted the open-num-flag branch November 23, 2015 01:39
Fishrock123 mentioned this pull request Nov 26, 2015
Closed
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
doc: clarify that fs streams expect blocking fd
41453b3 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
test: numeric flags to fs.open
0482ec4 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 30, 2015
doc: numeric flags to fs.open
d2d4f17 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
doc: clarify that fs streams expect blocking fd
b96f2a8 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
test: numeric flags to fs.open
660b688 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
doc: numeric flags to fs.open
511c1e7 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
doc: clarify that fs streams expect blocking fd
f4ca007 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
test: numeric flags to fs.open
f9fe0ae 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Dec 5, 2015
doc: numeric flags to fs.open
77251d9 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Dec 8, 2015
Closed
Merged
jasnell mentioned this pull request Dec 17, 2015
Closed
jasnell pushed a commit that referenced this pull request Dec 17, 2015
doc: clarify that fs streams expect blocking fd
5207099 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 17, 2015
test: numeric flags to fs.open
9a6c9fa 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 17, 2015
doc: numeric flags to fs.open
4c382e7 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
doc: clarify that fs streams expect blocking fd
bae03f2 
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 <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
test: numeric flags to fs.open
1570d32 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
doc: numeric flags to fs.open
b10433b 
This has been supperted for long but never tested nor documented.

PR-URL: #3641
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or 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
Show hidden characters
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

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL