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 options description for process.send() and worker.send() #29868

Closed
wants to merge 5 commits into from

Conversation

@dev-313
Copy link
Contributor

dev-313 commented Oct 7, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@dev-313

This comment has been minimized.

Copy link
Contributor Author

dev-313 commented Oct 7, 2019

This is my first PR, Please review. Thank you

@@ -467,6 +467,12 @@ changes:

* `message` {Object}
* `sendHandle` {Handle}
* `options` {Object} The `options` argument, if present, is an object used to

This comment has been minimized.

Copy link
@mscdex

mscdex Oct 7, 2019

Contributor

This is missing from the signature above

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 7, 2019

Author Contributor

@mscdex thanks, i fix it.

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 18, 2019

Author Contributor

@mscdex please can you give me some more idea about this.

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 21, 2019

Author Contributor

Hello @mscdex

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Oct 21, 2019

Member

@dev-313 - you need to modify the API signature at line 466 to add the options argument as well - like how did it for the worker.send API.

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 22, 2019

Author Contributor

@gireeshpunathil please review now.

This comment has been minimized.

Copy link
@gireeshpunathil

gireeshpunathil Oct 22, 2019

Member

looks good. @mscdex - PTAL!

package-lock.json Outdated Show resolved Hide resolved
@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from 3e9cf0c to 7ddaf06 Oct 7, 2019
@BridgeAR BridgeAR changed the title Added the description for the boolean return value of 'send()' in process.send() and worker.send() api. doc: add options description for process.send() and worker.send() Oct 9, 2019
@dev-313 dev-313 closed this Oct 10, 2019
@dev-313 dev-313 reopened this Oct 10, 2019
@dev-313

This comment has been minimized.

Copy link
Contributor Author

dev-313 commented Oct 10, 2019

@BridgeAR should I close this PR.

@dev-313 dev-313 closed this Oct 10, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 10, 2019

@dev-313 if I am not mistaken this is an documentation fix. As such, it would be great to keep it open. Seems like there's just one comment left that should be addressed (worker.send(message[, sendHandle][, callback]) misses the options argument completely and that should be added there).
I just do not know what options are valid and which are not for this API; @addaleax maybe?

@BridgeAR BridgeAR reopened this Oct 10, 2019
@@ -1939,7 +1939,12 @@ added: v0.5.9

* `message` {Object}
* `sendHandle` {net.Server|net.Socket}
* `options` {Object}
* `options` {Object} The `options` argument, if present, is an object used to

This comment has been minimized.

Copy link
@Trott

Trott Oct 14, 2019

Member
Suggested change
* `options` {Object} The `options` argument, if present, is an object used to
Object used to

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 15, 2019

Author Contributor

so should remove the existing options {Object} line and add only Object used to , Am i right @Trott

This comment has been minimized.

Copy link
@Trott

Trott Oct 15, 2019

Member

Whoops, my mistake. Let me try again.

Suggested change
* `options` {Object} The `options` argument, if present, is an object used to
* `options` {Object} Used to

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 15, 2019

Author Contributor

ok i understand @Trott , Thnaks

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 15, 2019

Author Contributor

@Trott i have one more doubt all three files have to change (child_process, cluster, process) or only process.md

This comment has been minimized.

Copy link
@Trott

Trott Oct 15, 2019

Member

Only process.md.

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 15, 2019

Author Contributor

Ok Thanks

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 14, 2019

@dev-313 I see you responded to a comment seven days ago, but it hasn't been addressed. Are you still working on this?

@dev-313

This comment has been minimized.

Copy link
Contributor Author

dev-313 commented Oct 15, 2019

yes @Trott i'm working on this.

@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from f220d41 to ffcf1ec Oct 15, 2019
@dev-313

This comment has been minimized.

Copy link
Contributor Author

dev-313 commented Oct 16, 2019

@Trott help why these stage 2 tests are failed.

@@ -1939,7 +1939,17 @@ added: v0.5.9

* `message` {Object}
* `sendHandle` {net.Server|net.Socket}
* `options` {Object}
<<<<<<< Updated upstream

This comment has been minimized.

Copy link
@Trott

Trott Oct 16, 2019

Member

Looks like you have a merge conflict to resolve.

This comment has been minimized.

Copy link
@dev-313

dev-313 Oct 16, 2019

Author Contributor

Thanks @Trott , I'll fix it,instead of this everything is ok or not.

@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from c586aaf to 10eef8e Oct 16, 2019
@nodejs-github-bot

This comment has been minimized.

@dev-313

This comment has been minimized.

Copy link
Contributor Author

dev-313 commented Oct 17, 2019

@Trott everytime i get this commit message error, please guide me how i can resolve this issue.
I know how to write a proper commit but confusion is that how to change this first commit message.

@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from 267da46 to eef460d Oct 17, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 18, 2019

@Trott everytime i get this commit message error, please guide me how i can resolve this issue.
I know how to write a proper commit but confusion is that how to change this first commit message.

You need to rebase and amend the first commit message. That can happen when the pull request is landed so if you don't get to it, that's OK. You do need to make changes to address @mscdex's comment, though.

dev-313 added 4 commits Oct 7, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from eef460d to 691ebd2 Oct 18, 2019
Add 'options' argument in worker.send() (doc/api/cluster.md)
API signature.

Fixes: #26995
@dev-313 dev-313 force-pushed the dev-313:add-devendra-shekhawat branch from 146fef7 to 0ccba0d Oct 21, 2019
@Trott

This comment has been minimized.

@dev-313 dev-313 requested a review from Trott Oct 22, 2019
@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Oct 23, 2019

all looks good to me. One more approval, and we should be ready to go!

@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Oct 30, 2019

invoking 7 day rule for PR with a single approval!

gireeshpunathil added a commit that referenced this pull request Oct 30, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Oct 30, 2019

landed as 5d1578d

thanks @dev-313 for your contribution!

targos added a commit that referenced this pull request Nov 5, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@targos targos mentioned this pull request Nov 5, 2019
targos added a commit that referenced this pull request Nov 8, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos added a commit that referenced this pull request Nov 10, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos added a commit that referenced this pull request Nov 10, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
targos added a commit that referenced this pull request Nov 11, 2019
Describes the meaning of the boolean return in process.send()
(doc/api/process.md) and worker.send() (doc/api/cluster.md) as
described in subprocess.send() (doc/api/child_process.md)

Fixes: #26995
PR-URL: #29868
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.