Update: max-params to only highlight function header

[ianobermiller]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

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:

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:

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:

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.

[matanwerbner]

This totally makes sense. working with this rule in the past, it was always strange to me that the whole function was highlighted, many times conflicting with other highlights, when the rule applies to the arguments list only.

I do feel like highlighting the whole params list could be better, since highlighting the first row alone might seem strange when many arguments are there.

[ilyavolodin]

Restarted Travis build. Thanks for PR! LGTM

[platinumazure]

LGTM, thanks for contributing!

I'm okay with this as is, but should we consider highlighting the parameters too?

[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 ***@***.***> wrote: ***@***.**** 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 <#10815 (review)>, 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 ***@***.***> 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 < ***@***.***> wrote: > ***@***.**** 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 > <#10815 (review)>, > 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?

[ilyavolodin]

@ianobermiller We will. We usually leave PRs open for 48 hours to give other members of the team a chance to look it over. Once we get closer to 48 hours, and no other comments, we will merge it in.

[ianobermiller]

Ah great, thanks for explaining. It’d be excellent to see that info in the PR guidelines.

…

On Thu, Aug 30, 2018 at 8:20 AM Ilya Volodin ***@***.***> wrote: @ianobermiller <https://github.com/ianobermiller> We will. We usually leave PRs open for 48 hours to give other members of the team a chance to look it over. Once we get closer to 48 hours, and no other comments, we will merge it in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10815 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2zi6m7rD0tE9D3nm0wVspoPvtaZ9l1ks5uWALMgaJpZM4WR5sY> .

[kaicataldo]

@ianobermiller Contributors don't have to request that PRs be merged - we'll take care of it!

For reference, the information @ilyavolodin referenced above is outlined here.

[ianobermiller]

Cool, thanks for linking that. I found this doc via the contributing file in the repo: https://eslint.org/docs/developer-guide/contributing/pull-requests I’d like to add a description of the process after you submit a PR, and link to the page you shared. I’ll send a PR for that!

…

On Sun, Sep 2, 2018 at 10:22 PM Kai Cataldo ***@***.***> wrote: @ianobermiller <https://github.com/ianobermiller> Contributors don't have to request that PRs be merged - we'll take care of it! For reference, the information @ilyavolodin <https://github.com/ilyavolodin> referenced above is outlined here <https://eslint.org/docs/maintainer-guide/pullrequests#when-to-merge-a-pull-request> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10815 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2ziw3cazirdM0ARLqiUUFEWj6wV0R_ks5uXLyLgaJpZM4WR5sY> .