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

doc: document and test that methods return this #13531

Merged
merged 1 commit into from
Jun 14, 2017
Merged

doc: document and test that methods return this #13531

merged 1 commit into from
Jun 14, 2017

Conversation

sam-github
Copy link
Contributor

Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

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)

doc,net

@sam-github sam-github requested a review from evanlucas June 7, 2017 20:07
@sam-github
Copy link
Contributor Author

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

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Jun 7, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jun 8, 2017

Looks like the CI had some issues with these changes.

@sam-github
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor Author

getConnections() doesn't return this either, I forgot and left in the test that proved it :-(. see #13553

I think that's the last of them.

@sam-github sam-github mentioned this pull request Jun 9, 2017
Merged
4 tasks
lpinca
lpinca reviewed Jun 10, 2017
View reviewed changes
doc/api/net.md Outdated
@@ -673,6 +693,8 @@ The numeric representation of the local port. For example,
Pauses the reading of data. That is, [`'data'`][] events will not be emitted.
Useful to throttle back an upload.

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.

Can you please add this also for socket.resume()?

@lpinca
Copy link
Member

lpinca commented Jun 10, 2017

I think makes sense to use 2 commits. One for the doc changes and one for the tests.

@refack
Copy link
Contributor

refack commented Jun 11, 2017

I count 11 (possibly 12) doc changes and only 5 test changes... sorry for not digging deeper but are the other cases covered (but where just undocumented)?

@sam-github
Copy link
Contributor Author

I'll audit again, but I believe the listen paths are well tested, our code is full of server.listen().on(....) call sequences.

@sam-github
Copy link
Contributor Author

not ok 985 parallel/test-net-write-after-close

Unrelated freebsd test failure, is that test flaky, @nodejs/platform-freebsd ?

@refack
Copy link
Contributor

refack commented Jun 11, 2017

not ok 985 parallel/test-net-write-after-close

Unrelated freebsd test failure, is that test flaky, @nodejs/platform-freebsd ?

Known: #13597

jasnell
jasnell requested changes Jun 12, 2017
View reviewed changes
doc/api/net.md
@@ -137,6 +137,8 @@ The optional `callback` will be called once the `'close'` event occurs. Unlike
that event, it will be called with an Error as its only argument if the server
was not open when it was closed.

Returns `server`.
Copy link
Member

Choose a reason for hiding this comment

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

In various other parts of the docs (randomly selected example: https://nodejs.org/dist/latest-v8.x/docs/api/crypto.html#crypto_certificate_exportchallenge_spkac) the Returns is listed as a bullet point along with the expected arguments. For consistency, this should do the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed existing docs to be like that, PTAL.

@sam-github
Copy link
Contributor Author

@jasnell I notice that streams has a different doc convention:

  • Returns: this

I'm not sure which is the convention we are heading towards, is this PR converging or diverging from where we want to go?

@gibfahn
Copy link
Member

gibfahn commented Jun 13, 2017

I'm not sure which is the convention we are heading towards, is this PR converging or diverging from where we want to go?

Sounds like another "do whatever in this PR, then raise a new PR unifying them and documenting it in the STYLE_GUIDE" question.

@sam-github
Copy link
Contributor Author

I'm OK to just get it right here, and agree, the STYLE_GUIDE should be updated.

@sam-github
Copy link
Contributor Author

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

@sam-github
doc: document and test that methods return this
44a8fa6 
Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

PR-URL: nodejs#13553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github merged commit 44a8fa6 into nodejs:master Jun 14, 2017
sam-github added a commit to sam-github/node that referenced this pull request Jun 14, 2017
@sam-github
doc: document and test that methods return this
84f3b8f 
Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

PR-URL: nodejs#13531
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jun 17, 2017
@sam-github @addaleax
doc: document and test that methods return this
397d5a7 
Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

PR-URL: #13531
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[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
doc: document and test that methods return this
58e177c 
Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

PR-URL: #13531
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jun 21, 2017
Merged
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@MylesBorins
Copy link
Contributor

I've ended up backporting this without the doc changes (as that was the conflict).

Please feel free to manually backport that change in a new PR

landed in 2fb1381

sam-github added a commit to sam-github/node that referenced this pull request Jul 21, 2017
@sam-github
doc: document methods that return this
3dd5282 
Backport of the documentation from
nodejs#13531 that did not land in 2fb1381.

PR-URL: nodejs#13553
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github sam-github mentioned this pull request Jul 21, 2017
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas Awaiting requested review from evanlucas

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @cjihrig @lpinca @refack @gibfahn @MylesBorins @jasnell @vsemozhetbyt