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.

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 example function isDead to cluster #28362

Closed
wants to merge 5 commits into from

Conversation

jessecogollo
Copy link
Contributor

@jessecogollo jessecogollo commented Jun 21, 2019

[x] added example documentation

[x]  documentation is changed or added
@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations. labels Jun 21, 2019
doc/api/cluster.md Outdated Show resolved Hide resolved
doc/api/cluster.md Outdated Show resolved Hide resolved
doc/api/cluster.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member

Trott commented Jun 21, 2019

@nodejs/documentation

Trott
Trott previously requested changes Jun 21, 2019
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Welcome @jessecogollo and thanks for the pull request! It may be possible to simplify/shorten this one a bit? It's probably fine the way it is though? I'm not sure so I pinged the documentation team. Thanks again!

@jessecogollo
Copy link
Contributor Author

Thank you @Trott.
I made the suggestions, let me know fi any change is necessary.

@Trott Trott dismissed their stale review June 22, 2019 02:35

changes implemented

process.kill(process.pid);
}).listen(8000);

// Make http://localhost:8000 to ckeck isDead method.
Copy link
Member

Choose a reason for hiding this comment

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

nit: typo

Suggested change
// Make http://localhost:8000 to ckeck isDead method.
// Make http://localhost:8000 to check isDead method.

Copy link
Member

Choose a reason for hiding this comment

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

"Make" means "Make a connection to" here?

Suggested change
// Make http://localhost:8000 to ckeck isDead method.
// Connect to http://localhost:8000 to check isDead().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Trott and @trivikr typo is fixed. Let me know other comment 😉

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM with typo fix. Seems that it could also be simplified/shortened a bit, but that's probably not a big priority. Thanks for the code!

@Trott
Copy link
Member

Trott commented Jun 25, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3713/

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 25, 2019
PR-URL: nodejs#28362
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@Trott
Copy link
Member

Trott commented Jun 25, 2019

Landed in e2d445b.

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@Trott Trott closed this Jun 25, 2019
targos pushed a commit that referenced this pull request Jul 2, 2019
PR-URL: #28362
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos mentioned this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants