Skip to content

net: return this from destroy()#13530

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
Merged
sam-github merged 1 commit into from
Jun 14, 2017
Merged

net: return this from destroy() #13530

sam-github merged 1 commit into from
Jun 14, 2017

Conversation

@sam-github
Copy link
Contributor

@sam-githubsam-github commented Jun 7, 2017

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
Affected core subsystem(s)

net,doc

@sam-github
Copy link
ContributorAuthor

sam-github commented Jun 7, 2017

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

@sam-githubsam-github added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 7, 2017
@gibfahn
Copy link
Member

gibfahn commented Jun 7, 2017

What's the reason behind this change?

Also do you need to return this in Line 17? I assume you're not doing that because it's an error handler.

LGTM otherwise.

@sam-github
Copy link
ContributorAuthor

sam-github commented Jun 8, 2017

What's the reason behind this change?

Method chaining, that's how I found the .end() not returning this, I tried to do .end().on(..., and it blew up. the destroy() I found by auditing.

Also do you need to return this in Line 17?

I do, that is a bug, thank you.

cjihrig
cjihrig approved these changes Jun 8, 2017
View reviewed changes
Copy link
Contributor

@cjihrigcjihrig 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 nit addressed.

mhdawson
mhdawson approved these changes Jun 8, 2017
View reviewed changes
Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@sam-githubsam-githubforce-pushed the socket-destroy-return-this branch from 6d4b1ad to 1fd74f9CompareJune 8, 2017 22:10
@sam-github
Copy link
ContributorAuthor

sam-github commented Jun 8, 2017

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

mcollina
mcollina approved these changes Jun 9, 2017
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

Can you move the test inside one of the stream-*-destroy files, and rename the commit and PR as "streams: ".

Good spot!

cc @nodejs/streams

TimothyGu
TimothyGu reviewed Jun 9, 2017
View reviewed changes
doc/api/net.md Outdated
Copy link
Member

@TimothyGuTimothyGuJun 9, 2017

Choose a reason for hiding this comment

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

Just to be consistent, maybe a

* Returns:{net.Socket}

should be added too for metadata?

Copy link
ContributorAuthor

@sam-githubsam-githubJun 9, 2017

Choose a reason for hiding this comment

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

Ouph, there isn't much consistency anywhere in net.md on how returns are documented. I'll update #13531 to fix the inconsistencies, and then update this one.

@mcollina
Copy link
Member

mcollina commented Jun 9, 2017

Can you also add it to the streams doc?

jasnell
jasnell requested changes Jun 13, 2017
View reviewed changes
doc/api/net.md Outdated
Copy link
Member

@jasnelljasnellJun 13, 2017

Choose a reason for hiding this comment

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

In other areas of the docs, the return is listed as a bullet point along with the arguments. This should follow that convention.

@sam-githubsam-githubforce-pushed the socket-destroy-return-this branch from 1fd74f9 to daded61CompareJune 13, 2017 20:46
@sam-github
Copy link
ContributorAuthor

sam-github commented Jun 13, 2017

@jasnell@mcollina PTAL

@sam-githubsam-githubforce-pushed the socket-destroy-return-this branch from 37c0a72 to 7be0be1CompareJune 14, 2017 15:50
@sam-github
Copy link
ContributorAuthor

sam-github commented Jun 14, 2017

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

@sam-github
net: return this from destroy()
cf24177
PR-URL: nodejs#13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
@sam-githubsam-githubforce-pushed the socket-destroy-return-this branch from 7be0be1 to cf24177CompareJune 14, 2017 20:16
@sam-githubsam-github merged commit cf24177 into nodejs:masterJun 14, 2017
@sam-githubsam-github deleted the socket-destroy-return-this branch June 14, 2017 20:17
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@sam-github@addaleax
net: return this from destroy()
3facee1
PR-URL: #13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
@addaleaxaddaleax mentioned this pull request Jun 17, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@sam-github@addaleax
net: return this from destroy()
fc06a8f
PR-URL: #13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
@sam-github@addaleax
net: return this from destroy()
c664e35
PR-URL: #13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
@sam-github@rvagg
net: return this from destroy()
0379174
PR-URL: #13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@sam-github@addaleax
net: return this from destroy()
69f806c
PR-URL: #13530 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jan 15, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@cjihrigcjihrigcjihrig approved these changes

@tniessentniessentniessen approved these changes

@mhdawsonmhdawsonmhdawson approved these changes

+4 more reviewers

@TimothyGuTimothyGuTimothyGu left review comments

@refackrefackrefack approved these changes

@Fishrock123Fishrock123Fishrock123 approved these changes

@gibfahngibfahngibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@sam-github@gibfahn@mcollina@refack@jasnell@Fishrock123@TimothyGu@cjihrig@tniessen@mhdawson