Doc fixes #9703

davidgilbertson · 2016-11-20T02:06:30Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

This fixes a number of issues in the documentation, originally logged as an issue here. As suggested by @bnoordhuis I have combined them in a single PR with 14 commits.

The relevant items (copied from the original issue I logged):

Should tls.connect() list host and port in 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() and path.parse() are in different orders, format() is logically wrong based on the diagram under parse()
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.Server shows connections as deprecated...
https://nodejs.org/api/net.html#net_server_connections
tls.Server inherits from this, but doesn't show connections as deprecated
https://nodejs.org/api/tls.html#tls_server_connections
Should it?
Is the encoding parameter allowed if the data parameter 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
dgram has 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
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() inside console.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.constants is 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 methods all have options as 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 Deflate object with an options."
By comparison, in net.connect() the equivalent wording is something like "The options are passed to the net.Socket constructor"
https://nodejs.org/api/zlib.html
The wording for there being Sync version of methods is different for fs and zlib.
https://nodejs.org/api/zlib.html#zlib_convenience_methods
https://nodejs.org/api/fs.html#fs_file_system

addaleax

One small request, otherwise this looks pretty good! Thanks!

addaleax · 2016-11-20T02:18:10Z

doc/api/tls.md

@@ -370,6 +370,7 @@ when the server has no more open connections.
 ### server.connections
 <!-- YAML
 added: v0.3.2
+deprecated: v0.9.7


Oooooh good catch!

addaleax · 2016-11-20T02:21:10Z

doc/api/vm.md


 // { animal: 'cat', count: 12, name: 'kitty' }
 ```

-### script.runInNewContext([sandbox][, options])
+### script.runInNewContext(sandbox[, options])


This should probably be [sandbox[, options]] – the sandbox is really optional, but options can only be passed when sandbox is, too

Thanks Anna. How should I handle this, just another commit?

@davidgilbertson If you’re git-savvy enough, I’d suggest changing the commit in this PR that made the original modification; i.e. git rebase -i master, replacing pick with edit for cb555d106c1edec7db2fe2ac6b96c088f0c92c36, then running the rebase.

You can also add another commit if that’s easier for you; generally, any way that makes a person who lands this PR understand what’s going on should be fine. :)

An hour later I can report back that I am not git-savvy enough. I've have just added another commit to make this change.

@davidgilbertson that’s perfectly okay… thanks for being so invested in this! :)

No problem. This is my first open source contribution, I'm having fun.

thefourtheye · 2016-11-20T06:30:51Z

doc/api/crypto.md

@@ -172,14 +172,14 @@ console.log(encrypted);
 // Prints: ca981be48e90867604588e75d04feabb63cc007a8f8ad89b10616ed84d815504
 ```

-### cipher.final([output_encoding])
+### cipher.final([outputEncoding])


All these changes have to be updated in the links at the bottom of this page.

Nice catch, thanks. I've updated the links in the footer. Actually I missed some other snake_case items on the page, I've fixed them up.

Also check links in other md files.

thefourtheye · 2016-11-20T06:36:36Z

doc/api/path.md

    base : "file.txt",
-    ext : ".txt",
-    name : "file"
+    name : "file",


This is an object, I don't think this change is necessary.

I'd agree not necessary, but these same five props are listed in five different places on the page, in different orders. The change just puts them all in the same order: dir, root, base, name, ext.

thefourtheye · 2016-11-20T06:58:31Z

doc/api/tls.md

@@ -789,8 +790,6 @@ added: v0.11.3
 * `port` {number}
 * `host` {string}
 * `options` {Object}
-  * `host` {string} Host the client should connect to.


This is still supported. We can pass host and port either directly or by setting them in options. In this case, both host and port are optional.

Oh OK. So, on that page, there is one signature tls.connect(options[, callback]) (which has port and host props) and then another one tls.connect(port[, host][, options][, callback]) (which is where this change is). So I thought showing port and host was redundant.

If they're valid I will add them back. Do you think it's worth documenting what would happen if you passed in a host, then an options object with a different host defined?

Yes, I think that behaviour should definitely be documented.

@Qard So ... what's the behaviour? Or should that be a separate PR, and if so, should I remove this change for now?

Yep, that can be a separate PR. I'd suggest just leaving the host and port fields in the options description for now.

I noticed this, too, I'll be PRing fixes to the tls.connect docs soon. Btw, options port/host overrides args port/host, which is opposite of what I would expect :-(

Qard · 2016-11-20T08:52:38Z

doc/api/zlib.md


-Every method has a `*Sync` counterpart, which accept the same arguments, but
-without a callback.


I would prefer this not be removed without the above additions explaining the distinction that the *Sync label is what indicates a method is synchronous. Your current wording could be misinterpreted as meaning that the exact same method could become synchronous by simply not giving it a callback.

This change was just to align it with the equivalent note about sync/async methods in fs which is currently:

All the methods have asynchronous and synchronous forms.

The asynchronous form always takes a completion callback as its last argument. The arguments passed to the completion callback ...

So the zlib text for the same would be (with the change in this PR):

All the methods have asynchronous and synchronous forms. The asynchronous form always takes a completion callback as its last argument in the form callback(error, result).

Note that just below this on the page the actual sync and non-sync methods are listed explicitly.

Does that change things? Otherwise, I can revert it to how it was.

Hmm...I suppose it's okay. I'd just like the sync vs async distinction to be clearer in docs in general. It's a major source of pain for new users, especially ones for which English is not their native language. I don't want to block this PR on that though.

Nothing beats an example, maybe (not this PR) a bare-bones comparison of fs.readFile() and fs.readFileSync() and the equivalent for zlib.

Fishrock123 · 2016-11-20T20:15:01Z

doc/api/crypto.md

@@ -172,14 +172,14 @@ console.log(encrypted);
 // Prints: ca981be48e90867604588e75d04feabb63cc007a8f8ad89b10616ed84d815504
 ```

-### cipher.final([output_encoding])
+### cipher.final([outputEncoding])


These changes break "hard links" and as such I do not think we should do this unless necessary.

I didn't realise that when I started, now I know I think you're quite right. It would be nice if everything was camelCase, but if everyone is happy for there to be this inconsistency then I'll revert this back.

Yeah, probably best the camel-casing be reverted.

Either that, or check all the links and update them, its easy to grep for what they used to be, though a bit painful to fix them all. Still, consistent camelCase would improve the docs, if you've the appetite for it.

I think the issue is the links out on the internet linking in to particular methods in the node docs. They would then just go to the top of the page.

@Fishrock123 I have removed these changes from the PR.

sam-github · 2016-11-21T17:16:03Z

doc/api/dgram.md

@@ -425,9 +425,7 @@ s.bind(1234, () => {
 });
 ```

-## `dgram` module functions


why? you just removed this header, and bumped out the level, so now all the dgram.* functions are documented as methods of the dgram.Socket class.

No this will have them listed at the same level as the classes. It's the same as http, https, readline, fs, etc. L1 heading is the module name, L2 is each class and each static (if that's the right word) method and property. L3 is for class instance methods/props.

dgram was the odd one out, this was just for consistency. Although, having just gone to check I didn't do something dumb, I notice that crypto also pushes all the module functions/props down to L3, but there is seems to make sense.

Show the encoding parameter as optional if the data parameter is passed in

Remove util.inspect() wrapper which is redundant inside console.log()

Fix [options] being parsed as a link and therefore not showing as an optional parameter (the brackets were stripped). Improved the clarity of the wording around passing of the options object

Add  to zlib.constants

Update the wording of the description of asynchronous and synchronous description for the zlib methods to match the description given for fs methods.

This lists the elements in the order dir, root, base, name, ext for path.format() and path.parse()

To make consistent with other pages, add the word "optional" to the description of path.resolve()

Remove spaces in tlsSocket.getPeerCertificate([ detailed ])

Remove the level-2 heading for "dgram module functions" and promote the two level-3 static methods to level-2 to align with the docs from other modules.

sandbox and options parameters were shown as independently optional which was not correct

davidgilbertson · 2016-11-27T07:03:19Z

@Fishrock123 I removed those changes that you commented on, are you able to approve?

italoacasas · 2017-01-16T21:20:31Z

ping @Fishrock123 can you give the green light on this?

ping @davidgilbertson you need to rebase in order to land this, thanks for the effort.

davidgilbertson · 2017-01-17T04:21:47Z

@italoacasas I've resolved the conflicts.

addaleax · 2017-01-17T04:28:59Z

@davidgilbertson Can you try to rebase this instead of merging master into your PR branch? Doing git rebase master (rather than git merge master) and then force-pushing should do the trick. If that doesn’t work out, that’s fine too, it would just be easier to keep the commits intact that way.

davidgilbertson · 2017-01-17T05:05:50Z

Ah OK my bad, I just tried github's new conflict-resolution-in-the-browser thing, I guess it does a merge.

I'll wait for fishrock then do it properly once.

addaleax · 2017-01-18T10:39:14Z

@davidgilbertson … looks like you may need to rebase once more anyway. (The conflict seems to be my fault, dc49838 vs 753021c – I’m pretty sure 7.0.0 is the correct version, though. Sorry!)

jasnell · 2017-03-24T22:55:59Z

Needs a rebase to continue

addaleax · 2017-04-29T22:55:18Z

Ping @davidgilbertson, can you rebase this again? :)

fhinkel · 2017-05-26T19:19:18Z

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 20, 2016

addaleax requested changes Nov 20, 2016

View reviewed changes

addaleax approved these changes Nov 20, 2016

View reviewed changes

thefourtheye reviewed Nov 20, 2016

View reviewed changes

Qard requested changes Nov 20, 2016

View reviewed changes

Fishrock123 suggested changes Nov 20, 2016

View reviewed changes

Qard approved these changes Nov 21, 2016

View reviewed changes

sam-github reviewed Nov 21, 2016

View reviewed changes

davidgilbertson added 12 commits November 23, 2016 17:22

doc: fix incorrect optional parameter nesting

6477814

Show the encoding parameter as optional if the data parameter is passed in

doc: remove util.inspect from events and vm docs

ad59eb6

Remove util.inspect() wrapper which is redundant inside console.log()

doc: fix incorrect linking of options parameter

08d4f88

Fix [options] being parsed as a link and therefore not showing as an optional parameter (the brackets were stripped). Improved the clarity of the wording around passing of the options object

doc: add missing apostrophe in vm docs

4b8c96c

doc: add version added to zlib.constants

dc49838

Add  to zlib.constants

doc: align wording of fs and zlib docs

c8e1a37

Update the wording of the description of asynchronous and synchronous description for the zlib methods to match the description given for fs methods.

doc: make order of path elements consistent

b735c77

This lists the elements in the order dir, root, base, name, ext for path.format() and path.parse()

doc: explicitly state path.resolve() param as optional

5b1414a

To make consistent with other pages, add the word "optional" to the description of path.resolve()

doc: mark tls.server#connections as deprecated

fac3950

doc: remove spaces around method parameter

34bf0dc

Remove spaces in tlsSocket.getPeerCertificate([ detailed ])

doc: correct header structure for dgram

a69ed0e

Remove the level-2 heading for "dgram module functions" and promote the two level-3 static methods to level-2 to align with the docs from other modules.

doc: fix optional parameters in runInNewContext()

9c16e77

sandbox and options parameters were shown as independently optional which was not correct

davidgilbertson force-pushed the doc-fixes branch from 83a1482 to 9c16e77 Compare November 23, 2016 06:37

silverwind approved these changes Nov 23, 2016

View reviewed changes

sam-github approved these changes Jan 17, 2017

View reviewed changes

Merge branch 'master' into doc-fixes

f60a32d

nodejs-github-bot added the dont-land-on-v7.x label Jan 17, 2017

italoacasas removed the dont-land-on-v7.x label Jan 18, 2017

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel closed this May 26, 2017

tniessen mentioned this pull request Jun 6, 2017

doc: minor issues #13491

Closed

2 tasks


		Every method has a `*Sync` counterpart, which accept the same arguments, but
		without a callback.

Doc fixes #9703

Doc fixes #9703

Conversation

davidgilbertson commented Nov 20, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgilbertson commented Nov 27, 2016

italoacasas commented Jan 16, 2017

davidgilbertson commented Jan 17, 2017

addaleax commented Jan 17, 2017

davidgilbertson commented Jan 17, 2017

addaleax commented Jan 18, 2017

jasnell commented Mar 24, 2017

addaleax commented Apr 29, 2017

fhinkel commented May 26, 2017