Add an exception to Metrics/ParameterLists.

[reitermarkus]

Add an exception for Metrics/ParameterLists when passing a lambda/proc to set_trace_func.

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Used the same coding conventions as the rest of the project.
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• All tests(rake spec) are passing.
• The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.
• Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

[Drenmi]

You could probably use a node matcher, like:

def_node_matcher :argument_to_set_trace_func?, <<-PATTERN
  ^^(send nil :set_trace_function _)
PATTERN

Each ^ ascends one level in the AST.

[reitermarkus]

Thanks, I will try that.

[bbatsov]

It might be better if something like this is configurable. I hate hardcoding exceptions this way.

[bbatsov]

I've been thinking about this a moment more and I came up with a couple of related notes:

• This is a good opportunity to improve the pretty bad documentation of this cop
• Why is this flagging lambas and procs to begin with? Seems it should flag only instance and class method definitions.

Don't know about the others, but I consider the current behavior a bug (that's why I added the note about documentation - right now it's hard to tell if this was intentional or not).

[Drenmi]

It might be better if something like this is configurable. I hate hardcoding exceptions this way.

Makes sense. Perhaps an IgnoreLambda option?

Why is this flagging lambas and procs to begin with? Seems it should flag only instance and class method definitions.

Good point. Could be an oversight, since both def and lambda/proc have a nested args node.

[reitermarkus]

Well, at least the name of the cop doesn't suggest that lamba/proc aren't covered by it, since they are still parameter lists. I wouldn't expect them to be excluded.

[bbatsov]

Well, at least the name of the cop doesn't suggest that lamba/proc aren't covered by it, since they are still parameter lists. I wouldn't expect them to be excluded.

But I know we created this cop with the intent to avoid people defining methods with many parameters. It was never the intention to check call sites, which is what your patch is addressing. Flagging those seems pretty pointless as often they are part of the API of some other method.

[bbatsov]

Good point. Could be an oversight, since both def and lambda/proc have a nested args node.

That's my assumption as well.

[bbatsov]

@reitermarkus Ping :-)

[reitermarkus]

I don't know what the plan is here now. Ignoring lambdas altogether?

[bbatsov]

Yeah, I'd just ignore lambdas completely.

[bbatsov]

The changes look OK. Just squash, rebase and add a changelog entry.

[bbatsov]

@reitermarkus Ping :-)

[reitermarkus]

Should be ready now.

[bbatsov]

This should normally go under Changes.

[bbatsov]

@reitermarkus I think you forgot to add yourself to the end of the changelog.

[reitermarkus]

I think you forgot to add yourself to the end of the changelog.

Ah! I couldn't figure out why my link wasn't working. 😄

[bbatsov]

👍