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

cli: add --prof flag description to node --help command #20845

Closed
wants to merge 2 commits into from
Closed

cli: add --prof flag description to node --help command #20845

wants to merge 2 commits into from

Conversation

ohbarye
Copy link
Contributor

@ohbarye ohbarye commented May 20, 2018

As suggested at #16459 (comment), I added description about --flag option into node --help command.

Closes: #16459

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [ ] tests and/or benchmarks are included I think it's necessary for this change
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 20, 2018
ohbarye
ohbarye commented May 20, 2018
View reviewed changes
doc/api/cli.md Outdated
@@ -240,6 +240,13 @@ resolving relative paths.

See `--preserve-symlinks` for more information.

### `--prof`
<!-- YAML
added: v5.2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct...? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I don't know the version it was introduced in, but it is much older than that. The flag already exists in 0.10.

Copy link
Member

Choose a reason for hiding this comment

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

I just checked and the log file is generated from v.2.0.0 on.

vsemozhetbyt
vsemozhetbyt reviewed May 20, 2018
View reviewed changes
doc/api/cli.md Outdated
added: v5.2.0
-->

Generate v8 profiler output.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: v8 -> V8.

doc/node.1 Outdated
@@ -149,6 +149,9 @@ Instructs the module loader to preserve symbolic links when resolving and cachin
.It Fl -preserve-symlinks-main
Instructs the module loader to preserve symbolic links when resolving and caching the main module.
.
.It Fl -prof
Generate v8 profiler output.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto)

src/node.cc Outdated
@@ -3234,6 +3234,7 @@ static void PrintHelp() {
" --preserve-symlinks-main preserve symbolic links when resolving\n"
" the main module\n"
#endif
" --prof generate v8 profiler output\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. And maybe you can fix the same typo on the next line?

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency. performance Issues and PRs related to the performance of Node.js. labels May 20, 2018
@ohbarye ohbarye force-pushed the add-prof-to-help-command branch from 127aad4 to e3cc05e Compare May 20, 2018 10:43
@ohbarye
Copy link
Contributor Author

ohbarye commented May 20, 2018

@vsemozhetbyt Thanks for the review, I fixed typo and rebased commits.

@vsemozhetbyt
Copy link
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/14983/

@vsemozhetbyt

This comment has been minimized.

Sign in to view
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented May 20, 2018 •
edited
Loading

Rerun failed tasks:
https://ci.nodejs.org/job/node-test-commit-windows-fanned/18132/ (green)
https://ci.nodejs.org/job/node-test-commit-linux-containered/4570/ (green)
https://ci.nodejs.org/job/node-test-commit-linux/18903/ (strangely green with one fail)

mmarchini
mmarchini approved these changes May 21, 2018
View reviewed changes
Copy link
Contributor

@mmarchini mmarchini left a comment

Choose a reason for hiding this comment

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

Nit: probably should update the commit message to Fixes: https://github.com/nodejs/node/issues/16459, otherwise I think the issue won't be closed.

@ohbarye ohbarye force-pushed the add-prof-to-help-command branch from e3cc05e to 1e17a91 Compare May 21, 2018 14:17
@ohbarye
Copy link
Contributor Author

ohbarye commented May 21, 2018

@mmarchini
Thanks for pointing it out. I updated the commit message.

@vsemozhetbyt
Copy link
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/739/

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 21, 2018
jasnell pushed a commit that referenced this pull request May 23, 2018
@ohbarye @jasnell
src,doc: add doc of --prof flag to help command
9461f32 
Fixes: #16459

PR-URL: #20845
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented May 23, 2018

Landed in 9461f32 with corrected metadata and revised commit message

@jasnell jasnell closed this May 23, 2018
@ohbarye
Copy link
Contributor Author

ohbarye commented May 23, 2018

Thank you node committers!

@ohbarye ohbarye deleted the add-prof-to-help-command branch May 23, 2018 16:15
targos pushed a commit that referenced this pull request May 25, 2018
@ohbarye @targos
src,doc: add doc of --prof flag to help command
b0023d7 
Fixes: #16459

PR-URL: #20845
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@targos targos targos left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@mmarchini mmarchini mmarchini approved these changes

@jasnell jasnell jasnell 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. c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

--prof flag not documented in --help output
8 participants
@ohbarye @vsemozhetbyt @jasnell @targos @mmarchini @gireeshpunathil @BridgeAR @nodejs-github-bot