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

a couple doc improvements / fixes #17702

Merged
merged 4 commits into from
Dec 18, 2017
Merged

Conversation

Fishrock123
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Mostly an edit pass over documentation.md but also some fixes to tty.md, fs.md, and synopsis.md.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Dec 15, 2017
is not recommended in production environments. Experimental features are not
subject to the Node.js Semantic Versioning model.
is not recommended in production environments. **Experimental features are not
subject to the Node.js Semantic Versioning model.**
Copy link
Member

Choose a reason for hiding this comment

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

I'm generally opposed to adding bold text except in extremely compelling situations. We waaaaaaay overuse typographical emphasis to the point that it has become mostly meaningless.

I'm especially opposed to it here because this is jargon a lot of people won't understand. I know you didn't write this text here, but "are not subject to the Node.js Semantic Versioning model" is vague and kind of misleading. There's nothing Node.js-specific about semantic versioning, and it's not clear that it could correctly be called a model or what that means/implies.

This text is redundant anyway. The very first sentence says things plainly: "subject to non-backwards compatible changes, or even removal, in any future release." Maybe dispense with this last jargon-y sentence and (if we must emphasize something) emphasize that text instead?

Copy link
Member

Choose a reason for hiding this comment

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

^^^^ non-blocking objection, by the way. Just comments.

```txt
Stability: 2 - Stable
The API has proven satisfactory. Compatibility with the npm ecosystem
is a high priority, and will not be broken unless absolutely necessary.
```

*Note*: Caution must be used when making use of `Experimental` features,
Copy link
Member

Choose a reason for hiding this comment

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

(Non-blocking) I'd remove *Note*: here and save it for things that really truly need it. Also remove the **Note**: (note different typography 🙄 ) later in this doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it worth keeping on that one.


## Syscalls and man pages

System calls like open(2) and read(2) define the interface between user programs
and the underlying operating system. Node functions which simply wrap a syscall,
like `fs.open()`, will document that. The docs link to the corresponding man
like [`fs.open()`][], will document that. The docs link to the corresponding man
pages (short for manual pages) which describe how the syscalls work.

**Note:** some syscalls, like lchown(2), are BSD-specific. That means, for
Copy link
Member

Choose a reason for hiding this comment

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

While in here, some -> Some

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I left some nits/opinions, but overall, seems like an improvement whether or not those nits/opinions are accepted. 😆

[`net.Socket`]: net.html#net_class_net_socket
[`process.stdin`]: process.html#process_process_stdin
[`process.stdout`]: process.html#process_process_stdout
[`process.stderr`]: process.html#process_process_stderr
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: out of ABC order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have we actually been enforcing this? I'm kinda clueless - if we have been generally I'll change it, if not... maybe we should do that separately with linting?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Dec 16, 2017

Choose a reason for hiding this comment

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

Sorry, I thought it was enforced in doc linting or style guide, but it seems I am wrong, I cannot even find PRs that sort references explicitly (I recall there were some). So maybe it is OK as is.

Copy link
Contributor Author

@Fishrock123 Fishrock123 Dec 16, 2017

Choose a reason for hiding this comment

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

For posterity: these are ordered (pretty sure I got it right) in their order of references.

@Fishrock123
Copy link
Contributor Author

Fishrock123 commented Dec 16, 2017

Lint CI: Edit, oops, again: https://ci.nodejs.org/job/node-test-linter/14546/

Reworded some parts, inter-doc links.

PR-URL: nodejs#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Also works on directories.

PR-URL: nodejs#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@Fishrock123 Fishrock123 merged commit ce38c49 into nodejs:master Dec 18, 2017
@Fishrock123 Fishrock123 deleted the doc-fixes branch December 18, 2017 15:38
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
Reworded some parts, inter-doc links.

PR-URL: nodejs/node#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
Also works on directories.

PR-URL: nodejs/node#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
PR-URL: nodejs/node#17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Reworded some parts, inter-doc links.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
Also works on directories.

PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
PR-URL: #17702
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Vse Mozhet Byt <[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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants