-
Notifications
You must be signed in to change notification settings - Fork 87
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
deprecated.property raise error when object is empty #12
Comments
Well, this is made to deprecate your API, so the premise is you would know what your own API looks like. But I see you are deprecating the accessing/setting of a property on a general If you cannot link to an example since it's not open source, you can always paste example code in here. Basically, to me, it seems weird to want to deprecate a property on an |
P.S. The reason that function requires the property to exist, is because for us to properly re-define it, we have to know if it's configurable and if it's enumerable. |
ok , because the object is optional argument. here it is: https://github.com/snowyu/level-subkey/blob/master/shell.js#L79 |
Ok. I think you are using the API incorrectly. To me, it looks like what you want for that line is actually this: if (opts.prefix) deprecate('prefix option, use `path` instead.') |
|
Got it. the js is very flexible script language, I can not compel developer to use a specified Options object like java. IMO most properties on javascript are just in a simple json object. so deprecate.property seldom useful. On java it is useful enough. btw, I am too lazy to type the check code in deprecate statement repeatedly. |
In the code you linked to. if you removed the line if (opts.prefix && !opts.path) opts.path = opts.prefix then you would see how the deprecation message from |
Basically, your code should look like this: if (opts.prefix) {
deprecate('prefix option, use `path` instead.')
if (!opts.path) opts.path = opts.prefix
} |
I have wrapped it with this: function deprecatedPrefixOption(options) {
if (options.prefix) {
deprecate('prefix option, use `path` instead.')
if (!options.path) options.path = options.prefix
delete options.prefix
}
} But this can be extracted as a new feature to do, what is a perfect name? function deprecate.assignProperty(object, deprecatedProp, currentProp) {
if (object[deprecatedProp]) {
deprecate(deprecatedProp+'property, use `'+currentProp+'` instead.')
if (!object[currentProp]) object[currentProp] = object[deprecatedProp]
delete object[deprecatedProp]
}
} |
That API is out of the scope of this module, unfortunately, as it's extremely opinionated in how you're supposed to use objects as Your helper also won't function correctly, because the call location of your |
Yes, the assignProperty helper function is just for assigning the current property to a depreacted property. even they do not wanna delete the deprecatedProp. Enhance a good API is not easy thing. Pardon? your means in the example code, should use this instead of deprecate function. |
I do not wanna to check the property of the options whether exists, why not do this in depd?
The text was updated successfully, but these errors were encountered: