-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat(rule): add valid-params rule #85
Conversation
@macklinu I can't remember if you're already a maintainer on this project or not, but I don't really have time to maintain this anymore. Do you want to to be added as an admin and given NPM access? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts 🗣
README.md
Outdated
* `Promise.reject()` is called with 2+ arguments | ||
* `Promise.then()` is called with 0 or 3+ arguments | ||
* `Promise.catch()` is called with 0 or 2+ arguments | ||
* `Promise.finally()` is called with 0 or 2+ arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got a little tired of writing the code examples by this point. 😂
Let me know if it would be better to include invalid code examples for each item in this list (similar to the Valid section above).
index.js
Outdated
@@ -32,7 +35,8 @@ module.exports = { | |||
'promise/no-promise-in-callback': 'warn', | |||
'promise/no-callback-in-promise': 'warn', | |||
'promise/avoid-new': 'warn', | |||
'promise/no-return-in-finally': 'warn' | |||
'promise/no-return-in-finally': 'warn', | |||
'promise/valid-params': 'warn' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this to the recommended config as a warning. Thoughts on making this an error instead?
rules/valid-params.js
Outdated
meta: { | ||
docs: { | ||
description: '', | ||
url: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update this and force push up.
af43b9d
to
9b2384f
Compare
@xjamundx I'm not a maintainer currently but would be glad to have that access and contribute in that way. Thank you! 😄 |
Just accepted the invite for push access. Thank you @xjamundx! 🎉 |
9b2384f
to
ef4db9e
Compare
Sure @macklinu I'll grant you NPM access after a bit. Try to go through the existing issues. You can do whatever you want. And please add your name to the README as a contribute or maintainer. I only have about an hour a month to spend on stuff like this at the moment and have mostly moved on to using typed JS to solve my static analysis needs for JS |
ef4db9e
to
54bc2de
Compare
54c8e6e
to
febf5d1
Compare
I should probably give you NPM access now. What's your NPM username? |
npm username is macklinu |
add92f8
to
8dbec40
Compare
8dbec40
to
622d77f
Compare
I think this is ready to merge. Will leave this open until I'm ready to merge #108 and ship v3.7.0 early this week / in case anyone has feedback. |
Resolves #47
As of now this rule simply checks how many arguments were supplied to the various
Promise
functions. If the length of arguments is invalid, it will report that as a lint violation. See the tests for examples.There are a couple of other issues (#3, #60) that I could see being rolled into this
valid-params
rule. What are your thoughts on that? I think those could be separate PRs, but some feedback on those ideas being included in this rule / any rule would be great. 👍