Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

ban: Refactor and add new option format #2547

Merged
merged 7 commits into from May 31, 2017
Merged

ban: Refactor and add new option format #2547

merged 7 commits into from May 31, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Apr 11, 2017

PR checklist

Overview of change:

Refactor and simplify banRule.
[enhancement] ban new options format: allows to specify an optional explanation message for function bans, banning nested methods and using a wildcard for object of a method ban
Fixes: #1940
Fixes: #2821

Is there anything you'd like reviewers to focus on?

This removes some ancient rather unintentional behavior, where banning foo.bar() would also ban obj.foo.bar() and even fn().foo.bar().
I looked through the git history and PRs for the rule and it seems adding this "feature" was unintended.

CHANGELOG.md entry:

@adidahiya adidahiya requested a review from nchen63 April 12, 2017 05:16
["xit"],
["fit"],
"xit",
{"name": "fit"},
Copy link
Contributor

Choose a reason for hiding this comment

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

add test for message in object

if (typeof arg === "string") {
functions.push({name: arg});
} else if (Array.isArray(arg)) {
if (arg.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

handle arg.length == 0

"eval",
{name: "$", message: "please don't"},
["describe", "only"],
{object: "it", name: "only", message: "don't fucus tests"},
Copy link
Contributor

Choose a reason for hiding this comment

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

"fucus" -> "focus"?

@nchen63
Copy link
Contributor

nchen63 commented Apr 13, 2017

what if someone actually wanted to ban obj.foo.bar()?

@ajafff
Copy link
Contributor Author

ajafff commented Apr 13, 2017

what if someone actually wanted to ban obj.foo.bar()?

That's really prone to errors, because it would also ban obj2.foo.bar() which may not be the desired behavior. Maybe we could extend the config to take an array for the object property.
For the above example you could then write:

{"object": ["obj", "foo"], "name": "bar"}

@adidahiya
Copy link
Contributor

bump @ajafff, can you address @nchen63's comments?

@ajafff
Copy link
Contributor Author

ajafff commented May 24, 2017

@adidahiya I actually did that and messed up a rebase recently ... will fix that

@ajafff
Copy link
Contributor Author

ajafff commented May 24, 2017

While thinking about #2821 I realized that there shouldn't be an object property. Instead name should be an array like it was before. But instead of limiting it to 2 elements for specifying the qualified name of the banned property, it should be open ended.
That would allow what @JoshuaKGoldberg requested in #2821

{"name": ["chai", "assert", "equal"], "message": "Use strictEqual instead of equal"}

Any thoughts about adding a wildcard to address @nchen63's comment #2547 (comment) to ban a property globally?

{"name": ["*", "forEach"], "message": "Use a regular for loop instead"}
// bans [].forEach(...) as well as foo.forEach(...), foo.bar.forEach(...) and foo.bar.baz.forEach(...)

@adidahiya
Copy link
Contributor

But instead of limiting it to 2 elements for specifying the qualified name of the banned property, it should be open ended.

sgtm 👍

Any thoughts about adding a wildcard

also seems fine, as the user is making it explicit that they are engaging in potentially wild behavior 😂

@ajafff
Copy link
Contributor Author

ajafff commented May 30, 2017

PR updated to allow banning nested methods as requested in #2821 and added a wildcard for method bans

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add nested accessors under the ban rule? [ban] Optional explanation for banned globals
3 participants