-
Notifications
You must be signed in to change notification settings - Fork 28.9k
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
doc: add a cli options doc page #5787
Conversation
Specify ICU data load path. (overrides `NODE_ICU_DATA`) | ||
|
||
|
||
## ENVIRONMENT VARIABLES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be all caps?
LGTM with a couple comments. |
70ac750
to
5326d92
Compare
LGTM |
|
||
<!--type=misc--> | ||
|
||
Node.js come with a wide variety of CLI options with multiple ways to execute scripts, built-in debugging, and a handful of other helpers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Node.js comes with a wide variety of CLI options. These options expose multiple ways to execute scripts, built-in debugging, and other helpful runtime options.
Line wrapping is somewhat inconsistent. Perhaps wrap it at 80? |
5326d92
to
96f6b9a
Compare
|
||
<!--type=misc--> | ||
|
||
Node.js comes with a wide variety of CLI options. These options expose built-in debugging, multiple ways to execute scripts, and other helpful runtime options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisdickinson Worded it this way so that it didn't sound like "multiple" applied to everything.
@bengl Fixed. I forgot about that when copying from the man page. |
@Fishrock123 lines 5 and 154? |
96f6b9a
to
accc590
Compare
Oops, didn't notice my editor was wrapping there, thought I had inserted spaces. Fixed. |
Cool, LGTM! |
Yep, nice. LGTM. |
Awesome work on all the recent docs PRs lately @Fishrock123 ! |
|
||
<!--type=misc--> | ||
|
||
Node.js comes with a wide variety of CLI options. These options expose built-in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CLI options -> command line options
perhaps? Are we sure all the users are familiar with the term? Windows users in particular...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that the title would clarify it, but I suppose I can can do this.
LGTM with nits. To clarify none of the nits prevent the merge IMO and they can be addressed at a later point or now depending on @Fishrock123's opinion. |
I would have liked if some of this feedback was done when I wrote some of these in the man doc refactor. D: |
I'd be against the nits, since it will become inconsistent to |
To be clear - I'm in the same opinion of @eljefedelrodeodeljefe of land now merge later. |
ok landing this as-is and making a new updating PR I'm afraid for this there isn't a better way to keep it in sync, formatting differences and other nits vary quite a bit from the |
Actually, I'm going to add to the comment in https://github.com/nodejs/node/blob/master/src/node.cc#L3296 in this PR also, just to try to help keep them in sync. |
accc590
to
f10894b
Compare
Ci, since this now modifies node.cc with a comment: https://ci.nodejs.org/job/node-test-pull-request/1992/ |
(Also I added a comment about |
This page is mostly a mirror of the updated manual page. PR-URL: nodejs#5787 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell jasnell@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Robert Lindstädt <robert.lindstaedt@gmail.com>
f10894b
to
d0edabe
Compare
Oops. Landed in 91cf55b, will make a new PR to fix some of the docs in multiple spots like tomorrow. |
This page is mostly a mirror of the updated manual page. PR-URL: #5787 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell jasnell@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Robert Lindstädt <robert.lindstaedt@gmail.com>
This page is mostly a mirror of the updated manual page. PR-URL: #5787 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell jasnell@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Robert Lindstädt <robert.lindstaedt@gmail.com>
This page is mostly a mirror of the updated manual page. PR-URL: #5787 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell jasnell@gmail.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Robert Lindstädt <robert.lindstaedt@gmail.com>
Pull Request check-list
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
doc
Description of change
This page is mostly a mirror of the updated manual page. This came about as a result of this twitter conversation, and I realized there wasn't an easy way to get the options documentation from the website.
cc @nodejs/documentation
Edit: Rendered (but our css makes it look slightly different)