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

Ideas for rules #4

Closed
mik01aj opened this issue Oct 2, 2015 · 5 comments
Closed

Ideas for rules #4

mik01aj opened this issue Oct 2, 2015 · 5 comments

Comments

@mik01aj
Copy link

mik01aj commented Oct 2, 2015

FYI: I submitted some rule ideas to eslint-plugins/eslint-plugin-lodash#3.

Here are these that are (in my understanding) not covered by your plugin:

  • detect stupid callbacks (for all functions that support 3 kinds of callbacks - map, filter, find, ... and many many more)

    _.filter(stuff, function(obj) { return obj.field === 'foo'; }) --> _.filter(stuff, {field: 'foo'})

  • detect stupid usages of _.find inside loops (with loops, I mean each, map, filter and so on)

    In most cases it's better to use _.indexBy to create an index and then query stuff using that index, thus reducing complexity from O(n²) to O(n • log n).

  • detect conditions like obj.a === 'foo' && obj.b === 'bar' && obj.c === 'baz' and suggest to replace them by _.matches(obj, {a: 'foo', b: 'bar', c: 'baz'})

  • detect for (var i=0; i>c.length; i++) and suggest to use _.each or _.map instead (maybe only when the only usage of i is reading/writing c[i])

  • detect usages of _.each or _.filter when _.find would be simpler

  • detect usages of _.each or _.filter when _.every or _.any would be simpler

  • or maybe even detect the above patterns not only for _.each, but also for plain JS loops!

  • detect usage of aliases, and suggest to use the aliased function instead (example: _.extend --> _.assign)

  • detect chains like a && a.b && a.b.c && a.b.c.d and suggest to replace them with _.get, _.set or _.has

  • _.isArray(x) && _.isObject(x) is equivalent to _.isArray(x). There also more possible similar rules.

Btw, I think it would be great to somehow merge this project and https://github.com/eslint-plugins/eslint-plugin-lodash, they do the same thing.

@ganimomer
Copy link
Contributor

Hi,
Thanks for the input! About your suggestions:

  • preventing stupid callbacks is implemented in prop-shorthand and matches-prop-shorthand.
  • usages of aliases is implemented in preferred-alias.

I think these rules sound good and we'll implement them soon (or you're welcome to PR and beat us to it 😄):

  • prefer-matches: detect conditions like obj.a === 'foo' && obj.b === 'bar' && obj.c === 'baz' and suggest to replace them by _.matches(obj, {a: 'foo', b: 'bar', c: 'baz'})
  • prefer-path-accessor: detect chains like a && a.b && a.b.c && a.b.c.d and suggest to replace them with _.get, _.set or _.has

About the other ones:

  • _.find inside loops: not sure how common that error is, or how severe it could be (I'm guessing probably most cases don't have a large enough collection to matter)
  • detecting for loops: I'm not sure this is related to lodash.
  • every and some: I think this should be a native rule, not a lodash one.
  • find: this should probably also be implemented natively (since ES6 has Array.prototype.find)
  • _.is* functions: I'm not sure this case is common or severe enough to merit a rule, but you're welcome to PR if you think differently and we'll definitely review it.

Thanks!

@mik01aj
Copy link
Author

mik01aj commented Oct 6, 2015

Thanks for the detailed feedback! :) It's great to hear that you'll actually implement some of them. I'd like to implement some of the others, but I don't know when I'll have time to get my hands on it.

_.find inside loops: not sure how common that error is, or how severe it could be (I'm guessing probably most cases don't have a large enough collection to matter)

Possibly it's not so common. I just noticed this pattern in my coworkers' code several times.

every and some: I think this should be a native rule, not a lodash one.

It depends on whether you prefer to use lodash's functions or the native ones. The point is about pieces of code that create a list using _.filter just to check its length (so _.count would be simpler) or even just check if the list is not empty (so _.every or _.some would be simpler). In a way, it's similar to the rule you have, that suggests to use _.reject instead of _.filter when the condition is negated.

@ganimomer
Copy link
Contributor

Hi,
We've implemented the new rule prefer-get - to prefer _.get or _.has over chains like a && a.b && a.b.c.d in version 0.1.10.

@mik01aj
Copy link
Author

mik01aj commented Oct 8, 2015

Great! I'm happy to hear that! 👍

@ganimomer
Copy link
Contributor

Implemented the new rule prefer-matches in v0.1.15.
Again, thanks for the input!

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

2 participants