Skip to content

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

Closed
ohbarye wants to merge 2 commits intonodejs:masterfrom
ohbarye:add-prof-to-help-command
Closed

cli: add --prof flag description to node --help command#20845
ohbarye wants to merge 2 commits intonodejs:masterfrom
ohbarye:add-prof-to-help-command

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
Copy link
Contributor Author

@ohbarye ohbarye May 20, 2018

Choose a reason for hiding this comment

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

Correct...? 🤔

Copy link
Member

@targos targos May 20, 2018

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

@BridgeAR BridgeAR May 20, 2018

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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 20, 2018

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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 20, 2018

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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt May 20, 2018

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

vsemozhetbyt commented May 20, 2018

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

vsemozhetbyt commented May 21, 2018

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 <vsemozhetbyt@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <vsemozhetbyt@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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

@targos targos targos left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@jasnell jasnell jasnell approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+2 more reviewers

@mmarchini mmarchini mmarchini approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

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