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

prefer-matches should be prefer-isMatch #77

Closed
sevenseat opened this issue May 11, 2016 · 5 comments
Closed

prefer-matches should be prefer-isMatch #77

sevenseat opened this issue May 11, 2016 · 5 comments

Comments

@sevenseat
Copy link

The prefer-matches rule does not work as currently constructed. The function _.matches only takes one argument and returns a function. Whereas ._isMatch will take two arguments and return a boolean. I'm pretty sure that's what you're looking for.

Thanks so much for this terrific library.

@mik01aj
Copy link

mik01aj commented May 12, 2016

I think this is caused by my mistake when I originally proposed this rule in #4. 😊

@ganimomer
Copy link
Contributor

Huh, I thought maybe _.isMatch was introduced after this rule but it seems that it's been there since version 3.
That does seem like a better fit (except, since ESLint says rules should be kebab case, it should be prefer-is-match, like our rule prefer-is-nil).
And I think the most important part to change would be the error message, not the rule name.

@ganimomer
Copy link
Contributor

I only changed the error message for now and not the rule name, since removing a rule is a breaking change (anyone who uses the rule explicitly and not in a config would have to change it, or ESLint would fail).

@jfmengels
Copy link
Contributor

Migration path idea: You could rename the rule, and then add a new entry in the index.js file with the old name to link to the new file. That way people can migrate easily, and you can deprecate prefer-matches early without removing it.

It's a bit trickier in this repo since you automated the indexing, but doable nonetheless

// index.js
var rules = _.assign(
  _.zipObject(rules, rules.map(rule => require(`./rules/${rule}`))),
  {'prefer-matches': require('./rules/prefer-isMatch')}
);
module.exports = {
  rules: rules,
}

@ganimomer
Copy link
Contributor

Thanks for the offer, but that would mean that the rule would exist twice in the meantime, and every time a user violates it, they would get two errors with the same message per violation.

I think the best course of action is to leave this for now and put it on the list of things to change in the next major version.

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

No branches or pull requests

4 participants