doc: improve util._extend function#8187
Uh oh!
There was an error while loading. Please reload this page.
Conversation
thefourtheye added util thefourtheyeforce-pushed the doc-fix-util-extend branch from 044291a to a547db8Compare
doc/api/util.md Outdated
There was a problem hiding this comment.
@thefourtheye Do we need an example for deprecated function? its like warn users and teach them how to use it 😄
There was a problem hiding this comment.
True. But we have done that in log too
cjihrig commented Aug 19, 2016
Thanks for the PR, but I'm not sure that we should improve the docs for this function. |
cjihrig commented Aug 19, 2016
Although maybe updating the function signature is appropriate :-) |
mscdex commented Aug 19, 2016
-1 to all but the function signature change. |
jasnell commented Aug 19, 2016
Given the deprecated status, I agree that we likely shouldn't extend the documentation on this. |
thefourtheyeforce-pushed the doc-fix-util-extend branch from a547db8 to d2c8e9bCompare
thefourtheye commented Aug 20, 2016
Okay. Fixed the function signature alone now. Hope this is okay. |
doc/api/util.md Outdated
There was a problem hiding this comment.
Should be origin and add to keep consistent with source, see https://github.com/nodejs/node/blob/master/lib/util.js#L976
There was a problem hiding this comment.
why not util._extend(target, source) without obj suffix ? @yorkieorigin/add combination is confusing.
There was a problem hiding this comment.
Both look good to me, but consistence should be considered here.
There was a problem hiding this comment.
@yorkie In the case of origin/add pair, I have code in place to understand what the particular function does. But documentation is the face of the code and we can not expect user to read the code in case of doubt. Its my humble opinion. ( extend the target from source)
There was a problem hiding this comment.
@princejwesley Why do we not change the source to target/source, too?
jasnell commented Aug 21, 2016
LGTM |
thefourtheye commented Aug 23, 2016
@yorkie@princejwesley Renamed the parameter names, for clarity. PTAL. |
targos commented Aug 23, 2016
LGTM |
yorkie commented Aug 23, 2016
LGTM :-) |
jasnell commented Aug 23, 2016
mscdex commented Aug 23, 2016
LGTM |
jasnell commented Aug 23, 2016
@thefourtheye ... can you please rebase and update this? |
thefourtheyeforce-pushed the doc-fix-util-extend branch 2 times, most recently from 5e1a699 to 7aa1e22Comparethefourtheye commented Aug 24, 2016
@jasnell Done. I'll land this tomorrow, if there are no other suggestions. |
The function signature of `util._extend` is not intuitive and the documentation doesn't specify the necessary second parameter. This patch changes the parameter names in the code and the function params in doc. PR-URL: nodejs#8187 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Brian White <[email protected]>
thefourtheyeforce-pushed the doc-fix-util-extend branch from 7aa1e22 to a8c7221Comparethefourtheye commented Aug 26, 2016
Landed in b3e7ac2 |
The function signature of `util._extend` is not intuitive and the documentation doesn't specify the necessary second parameter. This patch changes the parameter names in the code and the function params in doc. PR-URL: #8187 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Brian White <[email protected]>
The function signature of `util._extend` is not intuitive and the documentation doesn't specify the necessary second parameter. This patch changes the parameter names in the code and the function params in doc. PR-URL: nodejs#8187 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Brian White <[email protected]>
The function signature of `util._extend` is not intuitive and the documentation doesn't specify the necessary second parameter. This patch changes the parameter names in the code and the function params in doc. PR-URL: #8187 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Brian White <[email protected]>
8 participants
Checklist
Affected core subsystem(s)
doc, util
Description of change
improve
util._extendfunction's documentation