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

Proposal: Maximum params #795

Closed
tunnckoCore opened this issue Feb 21, 2017 · 8 comments

Comments

@tunnckoCore
Copy link

commented Feb 21, 2017

Digging more and more in ESLint rules, i found some good ones which enforces good style and more readable code. This issue is for max-params rule - not fixable.

Suggested configuration.

{
  "max-params": "error"
}

The default max option of this rule is 3. In my ~3 years here, ~8 years development and tons of node packages and written functions i have maximum 3-5 cases where i have more than 3 arguments. If there is some need, anyone can add a disable comment. In anyway it is "best practice" for years, not only in node, so i don't believe there will have big system impact.

@feross feross added the enhancement label Feb 21, 2017

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

I can't see this passing ecosystem without massive breakage.
I have many functions that have 3-5 params.

Maybe lets be less restrictive initially and set it to be like maximum of 6-8 first.
I think that would be probably fair.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Hell, you'd be breaking on official node APIs: https://nodejs.org/api/buffer.html#buffer_buf_compare_target_targetstart_targetend_sourcestart_sourceend

Again, maybe 6 as a max could be OK for now... but check the ecosystem.

@tunnckoCore

This comment has been minimized.

Copy link
Author

commented Feb 22, 2017

If it is 6-8 it is too too much. It's ridiculous, when we have promises in mainstream and it is absolutely confirmed better practice to use something like options object or apply functional programming and composability. We also have destructing which this rule does not detect. Move to the future.

But yea, I'm at least for 5, no more.. it would be just absurd and it would be better to not set that rule at all.

As about that exact method of the buffer - that exact module is Stability: 2, so... yea, we can change that.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

But yea, I'm at least for 5, no more.. it would be just absurd and it would be better to not set that rule at all.

Not true. Enforcing some constant would mean we could very easily bring that value down over time and versions if it is so desired.

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

As about that exact method of the buffer - that exact module is Stability: 2, so... yea, we can change that.

What?
That API is succint, and easy to understand. An options object would be ridiculous when the context is so clear.

Honestly, I'm leaning towards a general NACK for this proposal.

@tunnckoCore

This comment has been minimized.

Copy link
Author

commented Feb 22, 2017

Enforcing some constant would mean we could very easily bring that value down over time and versions if it is so desired

Actually yea, agree with that.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 8, 2017

I'm -1 on this rule. One shouldn't write code with that many params, but we've tended to skip these rules based on random constant values like "no more than 80 chars per line" and "no more than 3 nested callbacks" because they're not easy to remember.

@feross

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

It seems there's a consensus here (based on 👍s from collaborators) that we should skip this rule.

@tunnckoCore Thanks for the proposal, hope the rationale makes sense to you.

@feross feross closed this Apr 4, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
3 participants
You can’t perform that action at this time.