Repository: eslint/eslint
Pull request #10815 (2018-08-29 16:26:17)

Update: max-params to only highlight function header
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[ ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[X] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

Update the max-params rule to only report the location of the method header. Some tools, like Nuclide, will otherwise highlight the entire body of the method, which makes the rule visually noisy in practice.

**What rule do you want to change?**

max-params

**Does this change cause the rule to produce more or fewer warnings?**

No.

**How will the change be implemented? (New option, new default behavior, etc.)?**

New location reporting.

**Please provide some example code that this change will affect:**

```js
function foo(a, b, c, d, e, f, g) {
  // long body
  // on multiple
  // lines
}
```

**What does the rule currently do for this code?**

Highlights the entire function:

```js
function foo(a, b, c, d, e, f, g) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  // long body
^^^^^^^^^^^^^^
  // on multiple
^^^^^^^^^^^^^^^^
  // lines
^^^^^^^^^^
}
^
```

**What will the rule do after it's changed?**

Highlights only the function header:

```js
function foo(a, b, c, d, e, f, g) {
^^^^^^^^^^^^
  // long body
  // on multiple
  // lines
}
```

I also considered highlighting the entire params list, but it is not really
necessary, and has more noise when the function has a multi-line param list.


**Is there anything you'd like reviewers to focus on?**

It is fairly simple, just look at the test.

ianobermiller: Travis failure appears to be random and unrelated.
ilyavolodin: Restarted Travis build. Thanks for PR! LGTM
ianobermiller: I did give a reason not to highlight the param list in the PR description;
it isn’t uncommon to have param lists with destructuring and types to span
10-20 lines, which is a lot of content to highlight. In theory I think it
is best to highlight them, but in practice it will probably be annoying.
On Wed, Aug 29, 2018 at 12:29 PM Kevin Partington <notifications@github.com>
wrote:

> *@platinumazure* approved this pull request.
>
> LGTM, thanks for contributing!
>
> I'm okay with this as is, but should we consider highlighting the
> parameters too?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/eslint/eslint/pull/10815#pullrequestreview-150719069>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA2zi4KwE2oxqldXFAjbI7lZcch_cHD7ks5uVuuggaJpZM4WR5sY>
> .
>

ianobermiller: Another reason in favor of the existing approach is that it reuses a helper
that we should use in other rules to cut down on highlighting.
On Wed, Aug 29, 2018 at 5:46 PM Ian Obermiller <ian@obermillers.com> wrote:

> I did give a reason not to highlight the param list in the PR description;
> it isn’t uncommon to have param lists with destructuring and types to span
> 10-20 lines, which is a lot of content to highlight. In theory I think it
> is best to highlight them, but in practice it will probably be annoying.
> On Wed, Aug 29, 2018 at 12:29 PM Kevin Partington <
> notifications@github.com> wrote:
>
>> *@platinumazure* approved this pull request.
>>
>> LGTM, thanks for contributing!
>>
>> I'm okay with this as is, but should we consider highlighting the
>> parameters too?
>>
>> —
>> You are receiving this because you authored the thread.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/eslint/eslint/pull/10815#pullrequestreview-150719069>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AA2zi4KwE2oxqldXFAjbI7lZcch_cHD7ks5uVuuggaJpZM4WR5sY>
>> .
>>
>

platinumazure: Works for me. Thanks for considering!
ianobermiller: Thanks for the reviews and discussion! May someone with write access please merge this PR?

Patch:
diff --git a/lib/rules/max-params.js b/lib/rules/max-params.js
index 902391b74..4089e8ae8 100644
--- a/lib/rules/max-params.js
+++ b/lib/rules/max-params.js
@@ -53,7 +53,7 @@ module.exports = {
     },
 
     create(context) {
-
+        const sourceCode = context.getSourceCode();
         const option = context.options[0];
         let numParams = 3;
 
@@ -76,6 +76,7 @@ module.exports = {
         function checkFunction(node) {
             if (node.params.length > numParams) {
                 context.report({
+                    loc: astUtils.getFunctionHeadLoc(node, sourceCode),
                     node,
                     message: "{{name}} has too many parameters ({{count}}). Maximum allowed is {{max}}.",
                     data: {


Test patch:
diff --git a/tests/lib/rules/max-params.js b/tests/lib/rules/max-params.js
index 3cb47044f..959f5d7b8 100644
--- a/tests/lib/rules/max-params.js
+++ b/tests/lib/rules/max-params.js
@@ -86,6 +86,20 @@ ruleTester.run("max-params", rule, {
                 message: "Function 'test' has too many parameters (3). Maximum allowed is 2.",
                 type: "FunctionDeclaration"
             }]
+        },
+
+        // Error location should not cover the entire function; just the name.
+        {
+            code: `function test(a, b, c) {
+              // Just to make it longer
+            }`,
+            options: [{ max: 2 }],
+            errors: [{
+                line: 1,
+                column: 1,
+                endLine: 1,
+                endColumn: 14
+            }]
         }
     ]
 });