A rule to disallow properties from being called on an object [$15]

[TheSavior]

We want to be able to lint that we don't check in code in our tests with

describe.(only|skip)
it.(only|skip)
assert.equal // we enforce ===, so we require assert.strictEqual

and a few others.

It would be nice to have a customizable rule like http://eslint.org/docs/rules/no-restricted-modules where we can configure it with the name of the object (if any), the function name, and an optional message to use to tell the user if that function is called.

There is a $15 open bounty on this issue. Add to the bounty at Bountysource.

[eslintbot]

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

1. The version of ESLint you are using (run eslint -v)
2. The source code that caused the problem
3. The configuration you're using (for the rule or your entire config file)
4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

1. The use case for the rule - what is it trying to prevent or flag?
2. Whether the rule is trying to prevent an error or is purely stylistic
3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

1. The problem you want to solve (don't mention the solution)
2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

[willklein]

I've written a rule just like that, I will try to share it over the weekend.

I don't know if it makes more sense as a core rule or a plugin. The rule on its own would not have any defaults, and to enable it you'd also need to provide disallowed keys.

[nzakas]

Yeah, I'm on the fence for inclusion in core - filtering on object properties is always dicey because many objects can have the same properties.

[IanVS]

Sounds to me like the perfect use-case for a plugin.

[TheSavior]

@nzakas, I'm not proposing blocking a specific property on all objects.

I wrote this rule for JSCS before I started using ESLint, and the configuration for the rule looks like:

"disallowObjectMethod": [{
    "object": "describe",
    "method": "only",
    "message": "Using describe.only is not allowed"
  }, {
    "object": "describe",
    "method": "skip",
    "message": "Using describe.skip is not allowed"
  }, {
    "object": "it",
    "method": "only",
    "message": "Using it.only is not allowed"
  }, {
    "object": "it",
    "method": "skip",
    "message": "Using it.skip is not allowed"
  }, {
    "object": "assert",
    "method": "equal",
    "message": "Use assert.strictEqual instead of assert.equal"
  }],

[willklein]

@TheSavior, I created eslint-plugin-disallow-methods to address this. Let me know if it works for you:

GitHub repo
NPM page

I used the same configuration you suggested above, without the message, for now. I can work on adding it later.

[nzakas]

@TheSavior ah, gotcha. That looks really cool and useful.

@willklein would you mind porting that to core?

[willklein]

I'll work on a PR, give me a couple days.

[xjamundx]

This is so great for things like _.extend which we're hoping to discourage 👍

[mrmckeb]

Hi @willklein, how did you go with this one? We're keen to add this into our project and wanted to know if you were still going ahead with the PR.

[willklein]

@mrmckeb, thanks for checking in. I've been meaning to get back to this, give me the weekend. 😅

[mrmckeb]

@willklein not a problem! Take as long as you need - it's great news that this is coming to ESLint.

[willklein]

I've finally found some free time, prepping a PR now. I should have it up tomorrow.

I decided to call it no-restricted-properties, and check for both property reads and method calls.

[mrmckeb]

Thanks @willklein, that's great. I was just discussing this with a colleague yesterday - coincidental timing.

[mrmckeb]

Hey @willklein, just a quick note on the name - no-restricted-properties sounds like turning this on would turn off all property restrictions, not set restrictions. I liked the original name better, but it's obviously up to you and the team at ESLint to decide on that. Maybe restrict-properties?

[IanVS]

I think @willklein's proposal follows our naming conventions. I see where you are coming from, @mrmckeb, but there is precedent for this type of rule name (i.e. no-restricted-modules, no-restricted-syntax, etc.). I'm 👍 on the name.

[willklein]

Thanks @IanVS, that's indeed what I was thinking. I really mulled this over (and the config schema) and tried to follow suite with other rules.

I have everything working now but the schema. I'm getting this error:

Error: rule-tester:
    Configuration for rule "no-restricted-properties" is invalid:
    Value "1,[object Object],[object Object]" has more items than allowed.

I tried a few things. I currently have:

module.exports.schema = [
    {
        "type": "object",
        "properties": {
            "object": { "type": "string" },
            "property": { "type": "string" }
        },
        "additionalProperties": true
    }
];

I want to be able to configure the rule like this:

"no-restricted-properties": [2, { "object": "someObject", "property": "disallowedProperty" }, { /* additional objects here */ }]

Any suggestions? Should I post the PR as a WIP?

[IanVS]

I think a better configuration would be:

"no-restricted-properties": [2, [{ "object": "someObject", "property": "disallowedProperty" }, { /* additional objects here */ }]]

Note the objects are in an array, this would help your schema problems.

[willklein]

Thanks, I think I started with that, and then tried to get clever. I think I saw another rule do something like what I was trying, but I can't remember it now.

I'll go with this and get it working!

[platinumazure]

@willklein I'm looking at the schema validation section (https://github.com/eslint/eslint/blob/master/lib/config-validator.js#L26-L58) and see that we have a special case for if the schema is an array of schema objects (which prepends the error level validation schema), but that if an object is specified it will interpret that as a full schema.

If you really want the variable-length flow, you would probably need to do the full schema:

module.exports.schema = {
    type: "array",
    items: [
        // Rule level
        // One of the property/object tuples
    ],
    "additionalItems": // More of the property/object tuples
};

Or something like that-- JSON Schema is not my forte. The problem with this approach is your rule would be vulnerable to a hypothetical change in the way off/warning/error settings for rules are specified.

Honestly you'll probably just want to have the inline array.

[willklein]

@gyandeeps, thanks for prodding. I had trouble in my local setup testing things, though everything might just work as is. I've been busy since but would like to see if I can get a PR this week. 😅

[TheSavior]

@willklein Any update on your rule?

[tommck]

I see a commit to @willklein's branch, but I don't see it getting merged. This would be useful

[willklein]

My day job gave me some time to work on this, I should have a PR today. Sorry though, I've just been more committed to other things in my free time.

[Mouvedia]

I think I saw another rule do something like what I was trying, but I can't remember it now.

@willklein space-before-function-paren

[originalfoo]

As per #5872 (comment) , adding this here:

Would it be possible to simplify the options object? For example:

// ...
  "no-restricted-properties": [ "error", [ {
    "objectName": {
      "propertyName1": "Error message 1",
      "propertyName2": "Error message 2"
    },
    "anotherObjectName": {
       // ...
    }, // etc
  } ] ],

[platinumazure]

Anyone want to work on this? I can work on it (possibly using @willklein's original PR), but I'm also trying to get some of the JSCS compatibility rules moving.

[TheSavior]

I've gone ahead and finished up the work that @willklein started. There is light at the end of the tunnel!

[TheSavior]

@willklein since this was a joint effort, I'm happy to donate the $15 bounty to a charity. Any recommendations? Planned Parenthood?

[nzakas]

You need to claim the Bounty first, then you can do whatever you want with it. Go to https://www.bountysource.com/issues/25204515-a-rule-to-disallow-properties-from-being-called-on-an-object to get started.

[TheSavior]

Yep. I didn't want to claim it until @willklein and I figured out what we wanted to do with it since it was a joint effort.

[willklein]

@TheSavior, thanks, that's a great idea and it's all up to you.

[TheSavior]

@willklein and I decided on Girls Who Code instead.

(It's amazing how many different companies took fees out of this)