Build: ESLint: forbid unused function parameters #4381
Conversation
timmywil
left a comment
There was a problem hiding this comment.
I use this in other projects.
dmethvin
left a comment
There was a problem hiding this comment.
Definitely a good idea. This combined with the no-global-jQuery in testing should help eliminate errors.
|
@gibson042 Are you OK with this change? @leobalter @trentmwillis @Krinkle From QUnit perspective, does that sound good to you (see the change in |
|
@mgol Disallowing unused earlier parameters I'm not sure is a good convention for code quality on its own merit. I do agree it makes a lot of sense here for jquery wrt AMD closures and such. But for other projects, I think the regular ESLint detection is enough. It seems like that would only adds extra overhead and maintenance cost over time (as well as potential confusion for new developers), without offering any real benefit, right? |
|
@Krinkle It depends. Where function parameters are not flexible, it doesn't do much besides force you to prefix the parameter name with underscore. But it does help clean up function signatures wherever they are flexible enough to be changed and the unused parameter can be removed. But I see your point that jQuery gets the most value from auditing the parameters for AMD loading and building. |
|
@Krinkle jQuery's AMD is the biggest beneficent of this new rule but we'll most likely migrate to ES modules so it will stop being so in the future. I see other gains, though - while, as @timmywil noted, functions bound by external contracts need the In my PR I only had to add the If you think this rule is not a good fit for QUnit, I may just leave an override here but we haven't been using overrides so far. I see QUnit is, though, and quite a lot at that; QUnit also uses the |
|
@mgol Yeah, makes sense to share with e.g. jquery migrate. Using the preset for that makes sense. I'd say don't worry about QUnit in that case, we can disable things as needed, no problem. Off-topic for here, but it might make sense to expose two presets within the |
|
For anyone reading this, GitHub has messed the order of the comments and some older comments display below the newer ones. |
|
My two cents: I don't think it would impact QUnit much, if it becomes a pain we can opt-out as @Krinkle mentions above. |
This rule requires all function parameters to be used, not just the last one. In cases where that's not possible as we need to match an external API, there's an escape hatch of prefixing an unused argument with _. This change makes it easier to catch unused AMD dependencies and unused parameters in internal functions the API of which we may change at will, among other things. Ref jquery/jquery#4381
|
I submitted jquery/eslint-config-jquery#14 for the config changes. Please review, after publishing an update I'll change this PR. |
This rule requires all function parameters to be used, not just the last one. In cases where that's not possible as we need to match an external API, there's an escape hatch of prefixing an unused argument with _. This change makes it easier to catch unused AMD dependencies and unused parameters in internal functions the API of which we may change at will, among other things. Closes #14 Ref jquery/jquery#4381
This commit requires all function parameters to be used, not just the last one. In cases where that's not possible as we need to match an external API, there's an escape hatch of prefixing an unused argument with `_`. This change makes it easier to catch unused AMD dependencies and unused parameters in internal functions the API of which we may change at will, among other things. Unused AMD dependencies have been removed as part of this commit.
1f01f32 to
0745fac
Compare
This commit requires all function parameters to be used, not just the last one. In cases where that's not possible as we need to match an external API, there's an escape hatch of prefixing an unused argument with `_`. This change makes it easier to catch unused AMD dependencies and unused parameters in internal functions the API of which we may change at will, among other things. Unused AMD dependencies have been removed as part of this commit. Closes gh-4381 (cherry-picked from 438b1a3)
|
Cherry-picked to |
Summary
This commit requires all function parameters to be used, not just the last one.
In cases where that's not possible as we need to match an external API, there's
an escape hatch of prefixing an unused argument with
_.This change makes it easier to catch unused AMD dependencies and unused
parameters in internal functions the API of which we may change at will, among
other things.
Unused AMD dependencies have been removed as part of this commit.
What do you think? If you agree this change is worthwhile, should we add that rule to
eslint-config-jquery?Checklist
New tests have been added to show the fix or feature worksIf needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com