Added IgnoredMethods configuration to Style/CombinableLoops

[dvandersluis]

Related to #8848, it probably makes sense to make Style/CombinableLoops use IgnoredMethods, since it's possible to have a method that matches the method name conditions without being combinable.

---

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).
• 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.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[marcandre]

Now that #8851 is merged, your example no longer works (methods have different arguments)...

[dvandersluis]

@marcandre oops, thanks! I had changed the test but not the example, I’ll fix it.

[dvandersluis]

@marcandre updated!

[marcandre]

Cool. Now why should these two validates_each not be combined?

[dvandersluis]

@marcandre definitely true that you could. I was trying to come up with an example of something you might not want to combine despite being able to, this is maybe not the best example though.

Overall for this I was just thinking based on the original issue that there is a use case to make this cop less restrictive without having to be manually disabled (because the cop just looks for a method that starts or ends with each but it’s conceivable that there are methods that match that aren’t enumerators).

[marcandre]

The patch is well written and not particularly complex, but I'd wait until we have someone providing an actual example of false positive. For example, had it been merged originally, we might not have had the report about different arguments...

…

On Mon, Oct 5, 2020, 07:54 Daniel Vandersluis ***@***.***> wrote: @marcandre <https://github.com/marcandre> definitely true that you could. I was trying to come up with an example of something you might not want to combine despite being able to, this is maybe not the best example though. Overall for this I was just thinking based on the original issue that there is a use case to make this cop less restrictive without having to be manually disabled (because the cop just looks for a method that starts or ends with each but it’s conceivable that there are methods that match that aren’t enumerators). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8852 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAIH2QR562HA5A4S2ODOZLSJGXWHANCNFSM4SDNANSA> .

[dvandersluis]

To be fair, we didn't have the report about different arguments, I discovered it while trying to replicate the original issue 😁. This PR was my first attempt at fixing it (since the example provided in the issue wasn't actually failing) which I split off once I discovered that there actually was a legit issue that could be fixed (the arguments).

I figured it could still be valuable so I split it out of my original branch and put it up by itself so we could have this exact discussion. I'm ok with closing this though if you don't think it's necessary!

[bbatsov]

I'm leaning towards closing the PR at this point, as I'm not sure it solves an actual problem. We can revisit it down the road if the need to do so arises.