src/doc: improve man page and --help by silverwind · Pull Request #10157 · nodejs/node · GitHub

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

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

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.

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.

@silverwind Could you take a look again?

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.)

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?

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.

Remove. I hope we deprecate it soon then.

@Fishrock123 LGTY?

@silverwind Seems good to me

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

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

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

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>

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

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

git cherry-pick ba4847e879424ad173289e8fb96cc86a09ee899b
git cherry-pick 2d23562588eebaf9a2a3d3987fe6e4ef63ab9500

@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>

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

src/doc: improve man page and --help by silverwind · Pull Request #10157 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

src/doc: improve man page and --help#10157silverwind wants to merge 1 commit intonodejs:masternodejs/node:masterfrom silverwind:env-vars-mansilverwind/node:env-vars-manCopy head branch name to clipboard

Conversation

silverwind commented Dec 6, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

silverwind commented Dec 6, 2016

Uh oh!

Fishrock123 Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Dec 16, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 16, 2016

Uh oh!

Trott Dec 17, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind commented Dec 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Dec 17, 2016

Uh oh!

silverwind commented Dec 18, 2016

Uh oh!

silverwind commented Dec 19, 2016

Uh oh!

Fishrock123 commented Dec 21, 2016

Uh oh!

silverwind commented Dec 22, 2016

Uh oh!

silverwind commented Dec 22, 2016

Uh oh!

silverwind commented Dec 22, 2016

Uh oh!

silverwind commented Dec 22, 2016

Uh oh!

evanlucas commented Jan 3, 2017

Uh oh!

silverwind commented Jan 10, 2017

Uh oh!

evanlucas commented Jan 10, 2017

Uh oh!

MylesBorins commented May 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

src/doc: improve man page and --help#10157
silverwind wants to merge 1 commit into
nodejs:masterfrom
silverwind:env-vars-man

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