New: no-restricted-properties rule (fixes #3218) #7017

TheSavior · 2016-08-30T02:03:37Z

What issue does this pull request address?
#3218

What changes did you make? (Give an overview)
Added a rule to disallow certain method calls on specific objects. For example describe.only in mocha.

Is there anything you'd like reviewers to focus on?
Nope, should be pretty straight forward.

Extra thanks to @willklein. This is essentially all his code. I just updated it to the new syntax and made it pass the tests.

mention-bot · 2016-08-30T02:03:38Z

@TheSavior, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @alberto and @mysticatea to be potential reviewers

eslintbot · 2016-08-30T02:03:38Z

LGTM

jquerybot · 2016-08-30T02:03:54Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

TheSavior · 2016-08-30T02:11:34Z

I have signed the CLA, but since the PR has a commit from @willklein, it requires him to sign it as well. @willklein If you get a chance to do that, it would be greatly appreciated!

willklein · 2016-08-30T02:18:37Z

Sorry, I completed the CLA months ago but not for the email on the commit. All set! Thanks for picking this up. :)

nzakas · 2016-08-30T18:35:20Z

docs/rules/no-restricted-properties.md

@@ -0,0 +1,73 @@
+# Disallow certain object properties (no-restricted-properties)


Nitpick: Lowercase "d" to start the heading (new style)

nzakas · 2016-08-30T18:39:21Z

This looks really good. Only comments are on the docs, just to get them inline with how we're doing documentation these days. If you can fix those up, I think this is ready to go.

eslintbot · 2016-08-30T22:30:33Z

LGTM

TheSavior · 2016-08-30T22:31:50Z

Thanks for the review. All addressed.

nzakas · 2016-08-31T17:43:36Z

Looks like you have some linting errors in the markdown file, can you take a look?

eslintbot · 2016-08-31T17:45:20Z

LGTM

TheSavior · 2016-08-31T17:46:20Z

Any idea why the CLA started failing?

Edit
Looks like the CLA Page is 404ing: http://contribute.jquery.org/CLA/status/?owner=eslint&repo=eslint&sha=a938c40e1445fe40224b4ce35461692e1c8ad1dc

eslintbot · 2016-08-31T17:56:39Z

LGTM

TheSavior · 2016-08-31T17:57:15Z

Rebuilding didn't solve the CLA issue. Is it something on the bot's side?

platinumazure · 2016-08-31T18:06:48Z

@TheSavior Yes, I think either the CLA bot or the CLA website is broken. @kborchers is already looking into this a different ESLint PR.

kborchers · 2016-08-31T20:21:57Z

CLA check has been fixed. Sorry about that all.

mysticatea · 2016-09-01T21:57:12Z

lib/rules/no-restricted-properties.js

+            };
+
+            return restrictions;
+        }, {});


I think we should use Map objects instead of plain objects.

/*eslint no-restricted-properties: [error, {object: "obj", property: "foo"}]*/ // should not warn. toString.toString obj.toString obj.valueOf

/*eslint no-restricted-properties: [error, {object: "obj", property: "__proto__"}]*/ // should not warn. obj.toString obj.valueOf

Wow, good catch.

This has been great. This is the first time I have used Map. This was a great use for it. 👍

eslintbot · 2016-09-02T05:34:49Z

LGTM

mysticatea · 2016-09-02T09:14:59Z

LGTM now 😄
I'd like someone else to verify.

ilyavolodin · 2016-09-02T13:56:54Z

lib/rules/no-restricted-properties.js

+        return {
+            MemberExpression(node) {
+                const objectName = node.object && node.object.name;
+                const propertyName = node.property && node.property.name;


This is only going to catch foo.bar notation. Should we also catch foo['bar'] as well?

I think @mysticatea had recently added a "get simple property name" method
to ast-utils, might want to hunt that down.

On Sep 2, 2016 8:56 AM, "Ilya Volodin" notifications@github.com wrote:

In lib/rules/no-restricted-properties.js
#7017 (comment):

if (!restrictions.has(objectName)) {

restrictions.set(objectName, new Map());

}

restrictions.get(objectName).set(propertyName, {

message: option.message

});

return restrictions;

}, new Map());

return {

MemberExpression(node) {

const objectName = node.object && node.object.name;

const propertyName = node.property && node.property.name;

This is only going to catch foo.bar notation. Should we also catch
foo['bar'] as well?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/eslint/eslint/pull/7017/files/576e4cc27d1d2f01961be20a41fa94df19a75b19#r77349937,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWekx4AnK_LRONX9DtdWUi0el5Plroks5qmCsngaJpZM4JwEv3
.

eslintbot · 2016-09-02T16:11:45Z

LGTM

nzakas · 2016-09-02T16:30:33Z

Lgtm. Thanks for picking this up @TheSavior !

dinoboff · 2016-09-14T10:14:02Z

Nice addition.

Why is it in the "Node.js and CommonJS" category?

vitorbal · 2016-09-14T10:25:18Z

@dinoboff this has since been fixed by #7118

dinoboff · 2016-09-14T11:24:46Z

Thanks.

willklein and others added 2 commits August 29, 2016 18:42

New: no-restricted-properties rule (fixes eslint#3218)

1e849a8

Updating to use the new format

0a548a8

jquerybot added the CLA: Error label Aug 30, 2016

jquerybot added CLA: Valid and removed CLA: Error labels Aug 30, 2016

nzakas reviewed Aug 30, 2016
View reviewed changes

Updating the docs

fc32de5

Fixing lint errors in the markdown

a938c40

jquerybot added CLA: Error and removed CLA: Valid labels Aug 31, 2016

Force Build

9e91326

jquerybot added the CLA: Valid label Aug 31, 2016

jquerybot removed the CLA: Error label Aug 31, 2016

mysticatea reviewed Sep 1, 2016
View reviewed changes

Using Map instead of an object

576e4cc

ilyavolodin reviewed Sep 2, 2016
View reviewed changes

Using the property name getter from astUtils

c7e2ca2

nzakas merged commit 46cb690 into eslint:master Sep 2, 2016

TheSavior deleted the issue3218 branch September 2, 2016 16:42

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: no-restricted-properties rule (fixes #3218) #7017

New: no-restricted-properties rule (fixes #3218) #7017

TheSavior commented Aug 30, 2016

mention-bot commented Aug 30, 2016

eslintbot commented Aug 30, 2016

jquerybot commented Aug 30, 2016

TheSavior commented Aug 30, 2016

willklein commented Aug 30, 2016

nzakas Aug 30, 2016

nzakas commented Aug 30, 2016

eslintbot commented Aug 30, 2016

TheSavior commented Aug 30, 2016

nzakas commented Aug 31, 2016

eslintbot commented Aug 31, 2016

TheSavior commented Aug 31, 2016 •

edited

eslintbot commented Aug 31, 2016

TheSavior commented Aug 31, 2016

platinumazure commented Aug 31, 2016

kborchers commented Aug 31, 2016

mysticatea Sep 1, 2016

TheSavior Sep 2, 2016

TheSavior Sep 2, 2016

eslintbot commented Sep 2, 2016

mysticatea commented Sep 2, 2016

ilyavolodin Sep 2, 2016

platinumazure Sep 2, 2016

eslintbot commented Sep 2, 2016

nzakas commented Sep 2, 2016

dinoboff commented Sep 14, 2016

vitorbal commented Sep 14, 2016

dinoboff commented Sep 14, 2016

		@@ -0,0 +1,73 @@
		# Disallow certain object properties (no-restricted-properties)

New: no-restricted-properties rule (fixes #3218) #7017

New: no-restricted-properties rule (fixes #3218) #7017

Conversation

TheSavior commented Aug 30, 2016

mention-bot commented Aug 30, 2016

eslintbot commented Aug 30, 2016

jquerybot commented Aug 30, 2016

TheSavior commented Aug 30, 2016

willklein commented Aug 30, 2016

nzakas Aug 30, 2016

Choose a reason for hiding this comment

nzakas commented Aug 30, 2016

eslintbot commented Aug 30, 2016

TheSavior commented Aug 30, 2016

nzakas commented Aug 31, 2016

eslintbot commented Aug 31, 2016

TheSavior commented Aug 31, 2016 • edited

eslintbot commented Aug 31, 2016

TheSavior commented Aug 31, 2016

platinumazure commented Aug 31, 2016

kborchers commented Aug 31, 2016

mysticatea Sep 1, 2016

Choose a reason for hiding this comment

TheSavior Sep 2, 2016

Choose a reason for hiding this comment

TheSavior Sep 2, 2016

Choose a reason for hiding this comment

eslintbot commented Sep 2, 2016

mysticatea commented Sep 2, 2016

ilyavolodin Sep 2, 2016

Choose a reason for hiding this comment

platinumazure Sep 2, 2016

Choose a reason for hiding this comment

eslintbot commented Sep 2, 2016

nzakas commented Sep 2, 2016

dinoboff commented Sep 14, 2016

vitorbal commented Sep 14, 2016

dinoboff commented Sep 14, 2016

TheSavior commented Aug 31, 2016 •

edited