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.

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

New: the prefer-reflect rule (fixes #2939) #2996

Merged
merged 1 commit into from Jul 17, 2015
Merged

New: the prefer-reflect rule (fixes #2939) #2996

merged 1 commit into from Jul 17, 2015

Conversation

@keithamus
Copy link
Contributor

@keithamus keithamus commented Jul 14, 2015

This PR adds a rule to warn when the user is using "effectively deprecated" methods - which Reflect.* replaces, as discussed in #2939. Fixes #2939.

As the docs/tests suggest, each section can be disabled - meaning the user can configure this how they need to, for example if they want to use reflect for all but the delete operator, then they can configure it as [2, "delete"].

Let me know if you need anything else from me to merge this.

Oh, also, so close!
screen shot 2015-07-15 at 00 22 52

]
},
{
code: "(function(){}).apply(null, 1, 2)",
Copy link
Member

@nzakas nzakas Jul 15, 2015

Choose a reason for hiding this comment

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

Apply only takes two arguments.

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Jul 15, 2015

Overall looks good. I would like to change the options to be more in line with how we've defined exceptions in other rules. I can see two possible options:

prefer-reflect: [2, { exceptions: [ "delete"]}]

prefer-reflect: [2, { delete: false}]

Kinda leaning towards the second, but could be convinced otherwise.

Loading

@michaelficarra
Copy link
Member

@michaelficarra michaelficarra commented Jul 15, 2015

@nzakas The second permits a tri-state with different falsey values (false and undefined) meaning different things. That's not a good interface in my opinion.

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 15, 2015

I went more inline with a ruleset similar to no-restricted-modules. I'm happy to change it to the first example you show though.

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 15, 2015

Rebased, fixed tests and changed rule options to be {exceptions: [...]} as per @nzakas instruction

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 15, 2015

io.js travis build seems to be stuck, but I cant stop or restart it: https://travis-ci.org/eslint/eslint/jobs/71037738

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 15, 2015

Failure is down to travis timing out trying to install iojs:

$ nvm install iojs
                                                                           0.4%
Your test run exceeded 120 minutes. 
One possible solution is to split up your test run.

Force pushed to trigger a new build. Should pass now.

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 15, 2015

Oops, forgot to update docs in respect to change to using {exceptions:[]} syntax.

Docs updated now. This should be good to go.

Loading


var exceptions = (context.options[0] || {}).exceptions || [];

function report(node, existing, substitute) {
Copy link
Member

@nzakas nzakas Jul 15, 2015

Choose a reason for hiding this comment

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

Please add JSDoc comments here

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 16, 2015

Added jsdoc block for report function.

@nzakas is there anything else you'd like me to change?

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 16, 2015

Fixed lint errors JSDoc.

Loading

},
{
code: "Object.isExtensible({})",
args: [2, { exceptions: "apply" }],
Copy link
Member

@nzakas nzakas Jul 16, 2015

Choose a reason for hiding this comment

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

Can you use options instead of args?

options: [{exceptions: "apply"}]

You can leave off the initial 2, and any place with args: 2 can just be removed.

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 16, 2015

All done @nzakas. Anything else?

Loading


If you want to use Reflect methods, but keep using the `delete` keyword, then your config would look like `prefer-reflect: [2, { exceptions: ["delete"]] }`.

These can be combined as much as you like. To make all methods exceptions (thereby rendering this rule useless), use `prefer-reflect: [2, { exceptions: ["apply }", "call]", "defineProperty", "getOwnPropertyDescriptor", "getPrototypeOf", "setPrototypeOf", "isExtensible", "getOwnPropertyNames", "preventExtensions", "delete"]`
Copy link
Member

@ilyavolodin ilyavolodin Jul 16, 2015

Choose a reason for hiding this comment

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

I think there's an extra closing brace inside the array and one missing at the end of the line.

Loading

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Jul 16, 2015

LGTM. Btw, awesome documentation!

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 16, 2015

😄 thanks @ilyavolodin

Loading


The `exceptions` option allows you to pass an array of methods names you'd like to continue to use in the old style.

For example if you wish to use all Reflect methods, except for `Function.prototype.apply` then your config would look like `prefer-reflect: [2, { exceptions: ["apply"]] }`.
Copy link
Member

@ilyavolodin ilyavolodin Jul 16, 2015

Choose a reason for hiding this comment

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

Closing square brace is in the wrong place.

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Jul 17, 2015

Can you rebase?

Loading

@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 17, 2015

Rebased @nzakas

Loading

@ilyavolodin
Copy link
Member

@ilyavolodin ilyavolodin commented Jul 17, 2015

Thanks @keithamus for sticking with this!

Loading

ilyavolodin added a commit that referenced this issue Jul 17, 2015
@ilyavolodin ilyavolodin merged commit fb33c63 into eslint:master Jul 17, 2015
2 checks passed
Loading
@keithamus
Copy link
Contributor Author

@keithamus keithamus commented Jul 17, 2015

🎉

Loading

@keithamus keithamus deleted the feature-prefer-reflect branch Jul 17, 2015
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants