doc: document and test that methods return this#13531
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sam-github commented Jun 7, 2017
vsemozhetbyt added doc 
cjihrig commented Jun 8, 2017
Looks like the CI had some issues with these changes. |
sam-github commented Jun 8, 2017
sam-github commented Jun 8, 2017
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. |
doc/api/net.md Outdated
There was a problem hiding this comment.
Can you please add this also for socket.resume()?
lpinca commented Jun 10, 2017
I think makes sense to use 2 commits. One for the doc changes and one for the tests. |
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 commented Jun 11, 2017
I'll audit again, but I believe the listen paths are well tested, our code is full of |
sam-github commented Jun 11, 2017
Unrelated freebsd test failure, is that test flaky, @nodejs/platform-freebsd ? |
refack commented Jun 11, 2017
Known: #13597 |
doc/api/net.md Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I changed existing docs to be like that, PTAL.
sam-github commented Jun 13, 2017
@jasnell I notice that streams has a different doc convention:
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 commented Jun 13, 2017
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 commented Jun 13, 2017
I'm OK to just get it right here, and agree, the STYLE_GUIDE should be updated. |
sam-github commented Jun 14, 2017
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]>
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]>
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]>
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]>
MylesBorins commented Jul 17, 2017
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 commented Jul 18, 2017
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 |
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]>
8 participants
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), orvcbuild test(Windows) passesAffected core subsystem(s)
doc,net