Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: improve util._extend function #8187

Closed
wants to merge 1 commit into from
Closed

doc: improve util._extend function #8187

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc, util

Description of change

improve util._extend function's documentation

@thefourtheye thefourtheye added util Issues and PRs related to the built-in util module. doc Issues and PRs related to the documentations. labels Aug 19, 2016
princejwesley
princejwesley reviewed Aug 19, 2016
View reviewed changes
doc/api/util.md
// { name: 'Node.js' }
console.log(util._extend({'name': 'Node', 'lang': 'js'}, {'name': 'Node.js'}));
// { name: 'Node.js', lang: 'js' }
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thefourtheye Do we need an example for deprecated function? its like warn users and teach them how to use it 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. But we have done that in log too

@cjihrig
Copy link
Contributor

cjihrig commented Aug 19, 2016

Thanks for the PR, but I'm not sure that we should improve the docs for this function.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 19, 2016

Although maybe updating the function signature is appropriate :-)

@mscdex
Copy link
Contributor

mscdex commented Aug 19, 2016

-1 to all but the function signature change.

@jasnell
Copy link
Member

jasnell commented Aug 19, 2016

Given the deprecated status, I agree that we likely shouldn't extend the documentation on this.

@thefourtheye
Copy link
Contributor Author

Okay. Fixed the function signature alone now. Hope this is okay.

yorkie
yorkie reviewed Aug 20, 2016
View reviewed changes
doc/api/util.md
@@ -666,7 +666,7 @@ Deprecated predecessor of `console.log`.

Deprecated predecessor of `console.log`.

### util._extend(obj)
### util._extend(targetObj, sourceObj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be origin and add to keep consistent with source, see https://github.com/nodejs/node/blob/master/lib/util.js#L976

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not util._extend(target, source) without obj suffix ? @yorkie origin/add combination is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both look good to me, but consistence should be considered here.

Copy link
Contributor

@princejwesley princejwesley Aug 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@princejwesley Why do we not change the source to target/source, too?

@jasnell
Copy link
Member

jasnell commented Aug 21, 2016

LGTM

@thefourtheye
Copy link
Contributor Author

@yorkie @princejwesley Renamed the parameter names, for clarity. PTAL.

@targos
Copy link
Member

targos commented Aug 23, 2016

LGTM

@yorkie
Copy link
Contributor

yorkie commented Aug 23, 2016

LGTM :-)

@jasnell
Copy link
Member

jasnell commented Aug 23, 2016

@cjihrig and @mscdex ... PTAL

@mscdex
Copy link
Contributor

mscdex commented Aug 23, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 23, 2016

@thefourtheye ... can you please rebase and update this?

@thefourtheye thefourtheye force-pushed the doc-fix-util-extend branch 2 times, most recently from 5e1a699 to 7aa1e22 Compare August 24, 2016 04:53
@thefourtheye
Copy link
Contributor Author

@jasnell Done. I'll land this tomorrow, if there are no other suggestions.

@thefourtheye
util: improve function signature of util._extend
a8c7221 
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]>
@thefourtheye
Copy link
Contributor Author

Landed in b3e7ac2

@thefourtheye thefourtheye deleted the doc-fix-util-extend branch August 26, 2016 13:29
thefourtheye added a commit that referenced this pull request Aug 26, 2016
@thefourtheye
util: improve function signature of util._extend
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]>
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Closed
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
@thefourtheye @Fishrock123
util: improve function signature of util._extend
39d1ee3 
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]>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
@thefourtheye @Fishrock123
util: improve function signature of util._extend
d67ece2 
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]>
@piascikj piascikj mentioned this pull request Oct 3, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@thefourtheye @cjihrig @mscdex @jasnell @targos @yorkie @princejwesley @MylesBorins