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

net: return this from destroy() #13530

Merged
merged 1 commit into from
Jun 14, 2017
Merged

net: return this from destroy() #13530

merged 1 commit into from
Jun 14, 2017

Conversation

sam-github
Copy link
Contributor

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

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

@sam-github sam-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
Contributor Author

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

@cjihrig cjihrig 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

@mhdawson mhdawson 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-github sam-github force-pushed the socket-destroy-return-this branch from 6d4b1ad to 1fd74f9 Compare June 8, 2017 22:10
@sam-github
Copy link
Contributor Author

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

@mcollina mcollina 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
@@ -632,6 +632,8 @@ case of errors (parse error or so).
If `exception` is specified, an [`'error'`][] event will be emitted and any
Copy link
Member

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

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
@@ -632,6 +632,8 @@ case of errors (parse error or so).
If `exception` is specified, an [`'error'`][] event will be emitted and any
listeners for that event will receive `exception` as an argument.

Returns `socket`.
Copy link
Member

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-github sam-github force-pushed the socket-destroy-return-this branch from 1fd74f9 to daded61 Compare June 13, 2017 20:46
@sam-github
Copy link
Contributor Author

@jasnell @mcollina PTAL

@sam-github sam-github force-pushed the socket-destroy-return-this branch from 37c0a72 to 7be0be1 Compare June 14, 2017 15:50
@sam-github
Copy link
Contributor Author

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-github sam-github force-pushed the socket-destroy-return-this branch from 7be0be1 to cf24177 Compare June 14, 2017 20:16
@sam-github sam-github merged commit cf24177 into nodejs:master Jun 14, 2017
@sam-github sam-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]>
@addaleax addaleax 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]>
@MylesBorins MylesBorins mentioned this pull request Jan 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
semver-minor PRs 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