-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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.
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
fs: remove unnecessary usage of .hasOwnProperty() #635
Conversation
maybe add a test for this to prevent regressions? |
ES6 style would be |
okay i'll change it to @domenic's recommendations. @vkurchatkin what tests, specifically? |
@domenic not the same thing. what if @jonathanong like, that you pass |
@domenic actually, would |
@vkurchatkin in ES2015,
|
Concrete examples: function f(x = 1, y = 2) {
console.log(x, y);
}
function g({ x = 1, y = 2 }) {
console.log(x, y);
}
f(); // 1, 2
f(undefined, undefined); // 1, 2
f(null, null); // null, null
g({ x: undefined, y: undefined }); // 1, 2
g({}); // 1, 2
g({ x: null, x: null }); // null, null |
@domenic thanks! we should do the same, guess. But I'm afraid that in same cases swapping |
I agree with @domenic that I don't see that as a problem either way, because the only variable that it would affect is |
What is different in ES6 that makes hasOwnProperty unnecessary? |
@piscisaureus it's not about ES6, I think. @jonathanong want's to get reed of function fn({ option1, encoding='utf8'}) {
} encoding will be |
@jonathanong ping |
@vkurchatkin sup? Which direction should I take this PR? |
|
options = util._extend({ | ||
highWaterMark: 64 * 1024 | ||
}, options || {}); | ||
options = Object.create(options || {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this because i realized that without it, all my .hasOwnProperty()
stuff is futile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine since ReadableStream
doesn't use .hasOwnProperty
@jonathanong I think something has gone wrong with your rebaseing. |
LGTM |
PR-URL: #635 Reviewed-By: Vladimir Kurchatkin <[email protected]>
Landed in 4d0329e |
Notable changes: * buffer: New `Buffer#indexOf()` method, modelled off `Array#indexOf()`. Accepts a String, Buffer or a Number. Strings are interpreted as UTF8. (Trevor Norris) #561 * fs: `options` object properties in `'fs'` methods no longer perform a `hasOwnProperty()` check, thereby allowing options objects to have prototype properties that apply. (Jonathan Ong) #635 * tls: A likely TLS memory leak was reported by PayPal. Some of the recent changes in stream_wrap appear to be to blame. The initial fix is in #1078, you can track the progress toward closing the leak at #1075 (Fedor Indutny). * npm: Upgrade npm to 2.7.0. See npm CHANGELOG.md: https://github.com/npm/npm/blob/master/CHANGELOG.md#v270-2015-02-26 for details including why this is a semver-minor when it could have been semver-major. * TC: Colin Ihrig (@cjihrig) resigned from the TC due to his desire to do more code and fewer meetings.
This commit does various changes to cleanup and prep the 8ff6b81 for merging, which include updating the commit to follow new codebase guidelines. The following are the changes: doc/api/http.markdown: - document options lib/http.js: - no changes lib/_http_server.js - changes regarding isObject (nodejs#647) and hasOwnProperty (nodejs#635) - take tls option rather than guessing based on options test/simple/test-https-from-http.js: - moved to parallel directory, crypto test, removed copyright banner test/parallel/test-http-server.js: - adjust for tls option
on a mission to remove all instances of
util._extend()
and allow options to have prototypes.AFAIK,
.hasOwnProperty()
is unnecessary here.