doc: add argument information for socket.destroy()#7238
doc: add argument information for socket.destroy()#7238Trott wants to merge 1 commit intonodejs:masterfrom
Conversation
Trott
added
doc
doc/api/net.md
Outdated
There was a problem hiding this comment.
Two bits:
- What is the expected type of the argument
- Either
reasonorerrorwould be better thanexception
There was a problem hiding this comment.
@jasnell I chose exception because that's what it is called in the code. If we want to change it, do we have to reach into the code to change it there too?
I don't like error for the document because then you have to explain that error will be passed as an argument to any listeners on the 'error' event, which can get confusing especially to new users. So I'd probably go with reason over error, but reason doesn't signal to the reader that it's an error the way exception does.
As for the type, it would seem to be expecting an Error object but it actually doesn't look like there's any enforcement in the code so you could probably send it anything at all. I mean, it's just an argument for a listener, so it's kinda sorta up to the end user, I suppose. Suggest that it should (rather than must) be an Error perhaps?
|
Related: #7137 |
|
@nodejs/documentation |
|
bump, especially on specifying the type. (See #7238 (comment) above. The code does not check for a type and any type could be useful in theory, at least. Or maybe |
|
Small doc improvement...review, anyone? @nodejs/collaborators |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
PR-URL: nodejs#7238 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
|
Landed in 92adbe4 |
PR-URL: #7238 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: #7238 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
6 participants
Checklist
Affected core subsystem(s)
doc net
Description of change