src/doc: improve man page and --help #10157

silverwind · 2016-12-06T21:39:47Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src, doc

Description of change

added missing environment variables
sort environment variables alphabetically
add some highlighting to the man page
remove stops from descriptions in --help for consistency
few other minor tweaks to --help

silverwind · 2016-12-06T21:59:00Z

Marking this as semver-minor as it documents a feature added in fd644f5.

Fishrock123 · 2016-12-06T23:15:16Z

doc/node.1

@@ -182,26 +182,34 @@ Specify ICU data load path. (overrides \fBNODE_ICU_DATA\fR)
 \',\'\-separated list of core modules that should print debug information.

 .TP
-.BR NODE_PATH =\fIpath\fR[:\fI...\fR]
-\':\'\-separated list of directories prefixed to the module search path.
+.BR NODE_DISABLE_COLORS =\fI1\fR


There's not really an authority on this but I think this isn't correct in the way we've been using it.

That is, we italicize what kind of thing we pass to it, not what it actually is. Since that is inconsistent with things that can take multiple values, I left it as just all bold since that is the only way it could ever work anyways.

Are you suggesting not highlighting the value at all? This is how the current release renders for me:

While it seems more common that man pages don't add arguments in the title, it doesn't seem wrong to me to have them highlighted like this:

I guess to me the difference is that for NODE_DISABLE_COLORS the "value" is what it actually must be, and for the others it describes what the value should be set to.

I'm not hugely against changing it, but I feel it is no less confusing, perhaps either way.

Ah I see what you mean. I still think it'be more consistent to still highlight the 1, maybe in the description below as well.

Fishrock123 · 2016-12-06T23:16:41Z

src/node.cc

 #endif
         "NODE_REPL_HISTORY        path to the persistent REPL history file\n"
+         "NODE_TTY_UNSAFE_ASYNC    set to 1 to enable async stdout and stderr\n"


Leave this out of the --help please. It requires a warning which is far too long and bother some for --help and besides, this option isn't even helpful.

I feel it has it's place here, even if discouraged. The UNSAFE should make it clear enough that it has a caveat and users can look up details in the man page or in the online docs.

Fishrock123 · 2016-12-16T19:14:53Z

@silverwind Could you take a look again?

Trott · 2016-12-17T05:37:15Z

doc/node.1

-.BR NODE_PATH =\fIpath\fR[:\fI...\fR]
-\':\'\-separated list of directories prefixed to the module search path.
+.BR NODE_DISABLE_COLORS =\fI1\fR
+When set to 1 colors will not be used in the REPL.


Nit: While you're in here editing the file anyway, can you add a comma after 1? (Makes it clear that the variable should be set to 1 and not 1 colors or something like that.)

silverwind · 2016-12-17T14:07:42Z

Fixed nit and highlighted some more values in the man page. Regarding NODE_TTY_UNSAFE_ASYNC in --help, I feel that it should be there for completeness sake. Any other opinions on that one?

Fishrock123 · 2016-12-17T17:07:17Z

I'll make a PR later to remove that env variable, I added it solely at the request of others backwards compat concerns and firmly believe it should not even exist. I have not seen any help requests where people need it either.

silverwind · 2016-12-18T06:14:14Z

Remove. I hope we deprecate it soon then.

silverwind · 2016-12-19T20:56:46Z

@Fishrock123 LGTY?

Fishrock123 · 2016-12-21T20:37:40Z

@silverwind Seems good to me

silverwind · 2016-12-22T05:53:55Z

Looks like ba4847e introduced two more switches that were only added to the command line help, not to the man page. We should really review those better.

- add missing environment variables to --help - add missing flags to man page - sort environment variables alphabetically - add some highlighting to the man page - remove stops from descriptions in --help for consistency - few other minor tweaks to --help

silverwind · 2016-12-22T06:14:05Z

Fixed merge conflicts and added --trace-events-enabled and --trace-event-categories to the man page.

silverwind · 2016-12-22T15:53:30Z

Going to land this later today unless there are objections. The descriptions for the two new flags were copied as-is from the docs.

silverwind · 2016-12-22T23:27:33Z

Thanks! Landed in 2d23562.

- add missing environment variables to --help - add missing flags to man page - sort environment variables alphabetically - add some highlighting to the man page - remove stops from descriptions in --help for consistency - few other minor tweaks to --help PR-URL: #10157 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

evanlucas · 2017-01-03T05:43:36Z

@silverwind this isn't landing cleanly on v7.x. Mind backporting to v7.x-staging?

silverwind · 2017-01-10T21:57:17Z

@evanlucas it will land clean if you apply ba4847e first, is that good enough?

git cherry-pick ba4847e879424ad173289e8fb96cc86a09ee899b
git cherry-pick 2d23562588eebaf9a2a3d3987fe6e4ef63ab9500

evanlucas · 2017-01-10T22:12:42Z

@silverwind unfortunately, I'm pretty sure that commit relies on some changes in v8 that are not in v7.x, so that commit would have to be backported as well.

- add missing environment variables to --help - add missing flags to man page - sort environment variables alphabetically - add some highlighting to the man page - remove stops from descriptions in --help for consistency - few other minor tweaks to --help PR-URL: nodejs#10157 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

- add missing environment variables to --help - add missing flags to man page - sort environment variables alphabetically - add some highlighting to the man page - remove stops from descriptions in --help for consistency - few other minor tweaks to --help PR-URL: #10157 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Italo A. Casas <me@italoacasas.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins · 2017-05-15T21:59:03Z

we've opted to not land this on v6.x for now. It can likely land as a patch as the semver minor documentation has already landed on v6.x

Please feel free to backport

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 6, 2016

silverwind added the doc Issues and PRs related to the documentations. label Dec 6, 2016

Fishrock123 self-assigned this Dec 6, 2016

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 6, 2016

jasnell approved these changes Dec 6, 2016

View reviewed changes

cjihrig approved these changes Dec 6, 2016

View reviewed changes

italoacasas approved these changes Dec 6, 2016

View reviewed changes

Fishrock123 suggested changes Dec 6, 2016

View reviewed changes

Trott reviewed Dec 17, 2016

View reviewed changes

silverwind force-pushed the env-vars-man branch from 9c83ece to 6ce934f Compare December 17, 2016 14:06

nodejs-github-bot added the lts-watch-v6.x label Dec 17, 2016

Fishrock123 approved these changes Dec 21, 2016

View reviewed changes

Fishrock123 mentioned this pull request Dec 21, 2016

tty: remove NODE_TTY_UNSAFE_ASYNC #10393

Closed

3 tasks

silverwind force-pushed the env-vars-man branch from 182e0ce to 01a75a4 Compare December 22, 2016 06:11

nodejs-github-bot added the dont-land-on-v7.x label Dec 22, 2016

Fishrock123 removed the dont-land-on-v7.x label Dec 22, 2016

silverwind closed this Dec 22, 2016

silverwind deleted the env-vars-man branch December 22, 2016 23:27

evanlucas added the dont-land-on-v7.x label Jan 3, 2017

silverwind mentioned this pull request Jan 10, 2017

(backport) src/doc: improve man page and --help #10730

Closed

italoacasas mentioned this pull request Jan 31, 2017

v7.5.0 proposal #11062

Merged

gibfahn mentioned this pull request Apr 24, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

31 tasks

sam-github removed the semver-minor PRs that contain new features and should be released in the next minor version. label May 15, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels May 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src/doc: improve man page and --help #10157

src/doc: improve man page and --help #10157

silverwind commented Dec 6, 2016

silverwind commented Dec 6, 2016

Fishrock123 Dec 6, 2016

silverwind Dec 6, 2016 •

edited

Loading

Fishrock123 Dec 16, 2016

silverwind Dec 16, 2016 •

edited

Loading

Fishrock123 Dec 6, 2016

silverwind Dec 16, 2016 •

edited

Loading

Fishrock123 commented Dec 16, 2016

Trott Dec 17, 2016

silverwind commented Dec 17, 2016 •

edited

Loading

Fishrock123 commented Dec 17, 2016

silverwind commented Dec 18, 2016

silverwind commented Dec 19, 2016

Fishrock123 commented Dec 21, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

evanlucas commented Jan 3, 2017

silverwind commented Jan 10, 2017

evanlucas commented Jan 10, 2017

MylesBorins commented May 15, 2017

src/doc: improve man page and --help #10157

src/doc: improve man page and --help #10157

Conversation

silverwind commented Dec 6, 2016

Checklist

Affected core subsystem(s)

Description of change

silverwind commented Dec 6, 2016

Fishrock123 Dec 6, 2016

Choose a reason for hiding this comment

silverwind Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 Dec 16, 2016

Choose a reason for hiding this comment

silverwind Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 Dec 6, 2016

Choose a reason for hiding this comment

silverwind Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 commented Dec 16, 2016

Trott Dec 17, 2016

Choose a reason for hiding this comment

silverwind commented Dec 17, 2016 • edited Loading

Fishrock123 commented Dec 17, 2016

silverwind commented Dec 18, 2016

silverwind commented Dec 19, 2016

Fishrock123 commented Dec 21, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

silverwind commented Dec 22, 2016

evanlucas commented Jan 3, 2017

silverwind commented Jan 10, 2017

evanlucas commented Jan 10, 2017

MylesBorins commented May 15, 2017

silverwind Dec 6, 2016 •

edited

Loading

silverwind Dec 16, 2016 •

edited

Loading

silverwind Dec 16, 2016 •

edited

Loading

silverwind commented Dec 17, 2016 •

edited

Loading