A collection of minor issues with the docs #9538
Comments
mscdex
added
doc
|
These all look valid and excellent feedback to me, though I didn't look at 3 closely, you didn't say what is wrong. Its a lot of work, but I would request you PR each fix individually. Well, perhaps the zlib could be 5 commits in a single PR. Commenting on a list like above is not easy with the github UI, commenting on 19 issues would've been much easier, but at this point, I think going directly to a PR, not an issue, would be faster for you, and faster to get review comments, and get these fixed. 11 is historical, not meaningful, should be fixed 10 is probably because the dgram API didn't used to accept strings |
|
And you can't be too picky about the docs, they should be as clear as possible. |
|
I have no opinions on style issues but as to the technical side of things:
Yes.
The last one is the correct one, yes.
I think it's okay even if it's by accident (dgram didn't grow string support until quite late.) Applications that send the same string over and over would incur a performance hit because of the string->buffer conversion with every call.
Yes, if it's the one-argument version.
No, it's not - the documentation is wrong.
I think a single big PR is acceptable as long as it's broken up in logical commits, i.e., one fix per commit. |
|
@davidgilbertson thanks for taking the time to open an issue with all of these! |
|
@georgiwe not right place, open an issue (or open a PR that improves the docs!). |
oath.md: make order of properties consistent tls.md: remove spaces in getPeerCertificate signature tls.md: add deprecation notice to server.connections http.md: fix signature of request.end crypto.md: change crypto parameters to camelCase vm.md: add missing apostrophe vm.md: fix signature of vm.runInNewContext zlib.md: improve description of zlib.createXYZ PR-URL: #13491 Ref: #9538 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]>
oath.md: make order of properties consistent tls.md: remove spaces in getPeerCertificate signature tls.md: add deprecation notice to server.connections http.md: fix signature of request.end crypto.md: change crypto parameters to camelCase vm.md: add missing apostrophe vm.md: fix signature of vm.runInNewContext zlib.md: improve description of zlib.createXYZ PR-URL: #13491 Ref: #9538 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]>
oath.md: make order of properties consistent tls.md: remove spaces in getPeerCertificate signature tls.md: add deprecation notice to server.connections http.md: fix signature of request.end crypto.md: change crypto parameters to camelCase vm.md: add missing apostrophe vm.md: fix signature of vm.runInNewContext zlib.md: improve description of zlib.createXYZ PR-URL: #13491 Ref: #9538 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
These appear to have been fixed. Please comment (or re-open) if I am mistaken! |
Hi, I've just finished reading the docs and made a few notes about some minor issues I found. I thought I'd dump them here, I can do a PR if appropriate. I might be misunderstanding some things so didn't want to jump in.
Should
tls.connect()listhostandportin the options if they're already params?https://nodejs.org/api/tls.html#tls_tls_connect_port_host_options_callback
Seems weird that
path.resolve()arguments are optional. Worth documenting that the method returns'.'if you pass nothing in?https://nodejs.org/api/path.html#path_path_join_paths
https://nodejs.org/api/path.html#path_path_resolve_paths
The params for
path.format()andpath.parse()are in different orders,format()is logically wrong based on the diagram underparse()https://nodejs.org/api/path.html#path_path_format_pathobject
https://nodejs.org/api/path.html#path_path_parse_path
There shouldn't be spaces in
tlsSocket.getPeerCertificate([ detailed ])https://nodejs.org/api/tls.html#tls_tlssocket_getpeercertificate_detailed
net.Servershowsconnectionsas deprecated...https://nodejs.org/api/net.html#net_server_connections
tls.Serverinherits from this, but doesn't showconnectionsas deprecatedhttps://nodejs.org/api/tls.html#tls_server_connections
Should it?
Is the
encodingparameter allowed if thedataparameter isn't set?Should: request.end([data][, encoding][, callback])
be: request.end([data[, encoding]][, callback])
?
https://nodejs.org/api/http.html#http_request_end_data_encoding_callback
dgramhas a header "dgram module functions" before the static methods, none of the other pages do this. Is the dgram module different, or is this just an inconsistency?https://nodejs.org/api/dgram.html#dgram_dgram_module_functions
dgram.createSocket()has two different signatures listed. One withtype, one withobject.https://nodejs.org/api/dgram.html#dgram_dgram_createsocket_type_callback
That's cool, but the same sort of thing in the
fsmodule where anoptionsparameter could be a string defining the encoding, or an object that has an encoding property and something else is shown as a single signature.https://nodejs.org/api/fs.html#fs_fs_readfile_file_options_callback
IMO they should both be like
dgram, that is,fs.readFile(file[, encoding], callback)should be listed in addition tofs.readFile(file[, options], callback).But that means lots to change on the fs page.
There is (very?) minor inconsistency in callback terminology. E.g. socket connect specifies a
connectListenerwhich is nice, it defines that this is a callback that is the same as 'listening' for theconnectevent.https://nodejs.org/api/net.html#net_socket_connect_options_connectlistener
But
server.listennames the parameter justcallback.https://nodejs.org/api/net.html#net_server_listen_options_callback
Personally my choice would be the 'on' prefix followed by the event name that would trigger it.
onConnect,onListeningetc. I think that would make understanding when the callbacks are called easy.The example of a
dgamsending message unnecessarily converts a string to a buffer, since it will be converted to a (utf8)Bufferanyway. Less complexity in examples is better, right?(Also it vaguely implies that a string isn't as valid as a buffer.)
https://nodejs.org/api/dgram.html#dgram_socket_send_msg_offset_length_port_address_callback
Parameter names are almost always camelCase, except in crypto where they're snake_case.
https://nodejs.org/api/crypto.html#crypto_cipher_final_output_encoding
Is that an inconsistency or is there meaning in that?
Using
util.inspect()insideconsole.log()is redundant (I think. Isn't it?)https://nodejs.org/api/events.html#events_emitter_listeners_eventname
https://nodejs.org/api/vm.html#vm_vm_runincontext_code_contextifiedsandbox_options
https://nodejs.org/api/vm.html#vm_script_runinnewcontext_sandbox_options
Missing apostrophe in "run using the vm modules methods"
https://nodejs.org/api/vm.html#vm_what_does_it_mean_to_contextify_an_object
I have no idea about this one, but is sandbox really optional in
script.runInNewContext()?If I call
script.runInNewContext(options), wouldn't it try and make a sandbox out of the options object? I went looking through the source but found myself in a strange land and gave up.https://nodejs.org/api/vm.html#vm_script_runinnewcontext_sandbox_options
zlib.constantsis missing "added in version..." (and is quite new so this is a problem)https://nodejs.org/api/zlib.html#zlib_zlib_constants
Looks like this commit
197a465#diff-c245d87dba893de0b77c7574f0081633R388
zlib.codesisn't documented. Is that on purpose?https://nodejs.org/api/zlib.html
zlibmethods all haveoptionsas an argument, but not shown in square brackets (but in the TOC they're shown in square brackets). Less important, but having the word in the argument list be linked is inconsistent with the other pages. The description contains the word 'options' which is linked to the class options section.Wording is strange for zlib methods, e.g. "Returns a new
Deflateobject with an options."By comparison, in
net.connect()the equivalent wording is something like "The options are passed to thenet.Socketconstructor"https://nodejs.org/api/zlib.html
Getting picky here ... but the wording for there being Sync version of methods is different for
fsandzlib.https://nodejs.org/api/zlib.html#zlib_convenience_methods
https://nodejs.org/api/fs.html#fs_file_system
The text was updated successfully, but these errors were encountered: