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

doc: fix some recent nits in fs.md #29906

Closed
wants to merge 1 commit into from
Closed

doc: fix some recent nits in fs.md #29906

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Oct 9, 2019 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
  • Fix sorting of sections.
  • Fix sorting of bottom references.
  • Wrap lines at 80 chars.
  • Fix a heading level.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Oct 9, 2019
@Fishrock123
Copy link
Contributor

This conflicts with #29890

Fishrock123
Fishrock123 previously requested changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Disagree with the *Sync ordering

doc/api/fs.md Outdated
@@ -2680,6 +2658,33 @@ The `encoding` option sets the encoding for the `path` while opening the
directory and subsequent read operations (unless otherwise overriden during
reads from the directory).

## fs.openSync(path[, flags, mode])
Copy link
Contributor

Choose a reason for hiding this comment

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

This ordering is wrong. The sync versions should directly follow the async versions or else this doc will become horribly confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, we never made exceptions in ABC order. See in the current doc:

fs.read(fd, buffer, offset, length, position, callback)
fs.readdir(path[, options], callback)
fs.readdirSync(path[, options])
fs.readFile(path[, options], callback)
fs.readFileSync(path[, options])
fs.readlink(path[, options], callback)
fs.readlinkSync(path[, options])
fs.readSync(fd, buffer, offset, length, position)

Or:

fs.write(fd, buffer[, offset[, length[, position]]], callback)
fs.write(fd, string[, position[, encoding]], callback)
fs.writeFile(file, data[, options], callback)
fs.writeFileSync(file, data[, options])
fs.writeSync(fd, buffer[, offset[, length[, position]]])
fs.writeSync(fd, string[, position[, encoding]])

Should we make an exception now?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems wrong to me, but... ok.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Oct 9, 2019 •
edited
Loading

@Fishrock123 Should I resolve the conflicts here or do you prefer to merge some parts to #29890 so I can close this PR?

@Fishrock123 Fishrock123 dismissed their stale review October 9, 2019 18:44

our docs structure is endlessly confusing

Fishrock123
Fishrock123 previously requested changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

@vsemozhetbyt Please rebase this onto my branch... and/or wait

(doing in request so no one goes to merge this prematurely..)

@Fishrock123 Fishrock123 mentioned this pull request Oct 9, 2019
Closed
3 tasks
@vsemozhetbyt
Copy link
Contributor Author

I'll wait till #29890 landed. Sorry for being part of endless doc confusing)

@Fishrock123 Fishrock123 dismissed their stale review October 10, 2019 07:16

Ok #29890 landed, this can be rebased. 😄

@vsemozhetbyt
Copy link
Contributor Author

Rebased)

@vsemozhetbyt
Copy link
Contributor Author

Rebased.

@Trott
Copy link
Member

Trott commented Oct 12, 2019

Needs another rebase but LGTM otherwise.

@vsemozhetbyt
doc: fix some recent nits in fs.md
f296e0c 
* Fix sorting of sections.
* Fix sorting of bottom references.
* Wrap lines at 80 chars.
* Fix a heading level.
@vsemozhetbyt
Copy link
Contributor Author

Rebased.

@Trott
Copy link
Member

Trott commented Oct 12, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3989/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2019
@Trott
Copy link
Member

Trott commented Oct 12, 2019

Landed in 039eb56

@Trott Trott closed this Oct 12, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 12, 2019
@vsemozhetbyt @Trott
doc: fix some recent nits in fs.md
039eb56 
* Fix sorting of sections.
* Fix sorting of bottom references.
* Wrap lines at 80 chars.
* Fix a heading level.

PR-URL: nodejs#29906
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@vsemozhetbyt vsemozhetbyt deleted the doc-fs-nits-2019-10-09 branch October 12, 2019 16:36
@BethGriggs BethGriggs mentioned this pull request Oct 21, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@vsemozhetbyt @targos
doc: fix some recent nits in fs.md
8c547b5 
* Fix sorting of sections.
* Fix sorting of bottom references.
* Wrap lines at 80 chars.
* Fix a heading level.

PR-URL: #29906
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2019
@vsemozhetbyt @targos
doc: fix some recent nits in fs.md
c66bc20 
* Fix sorting of sections.
* Fix sorting of bottom references.
* Wrap lines at 80 chars.
* Fix a heading level.

PR-URL: #29906
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Nov 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vsemozhetbyt @Fishrock123 @Trott @cjihrig @BridgeAR @nodejs-github-bot