Extract helper module to detect repeated nodes#2155
Conversation
| def find_repeated_groups(items, key_proc:) | ||
| items | ||
| .group_by(&key_proc) | ||
| .values | ||
| .reject(&:one?) | ||
| end |
There was a problem hiding this comment.
Wouldn't it be simpler to just use a block instead of passing a Proc instance as an argument?
| def find_repeated_groups(items, key_proc:) | |
| items | |
| .group_by(&key_proc) | |
| .values | |
| .reject(&:one?) | |
| end | |
| def find_repeated_groups(items, &block) | |
| items | |
| .group_by(&block) | |
| .values | |
| .reject(&:one?) | |
| end |
There was a problem hiding this comment.
I actually found the code a bit confusing, when I tried rewriting it with an implicit block just now. If we do do that however, could we rename the block argument from &block to e.g. &signature_proc?
There was a problem hiding this comment.
That makes sense.
Without keyword arguments to label things, it can be hard to tell what's going on from the caller’s side. In that case, the original way of passing a Proc via keyword arguments definitely has its perks. Alternatively, it might improve things somewhat to change the method name to something like find_repeated_groups_by, which clearly implies that grouping logic is expected.
Since both approaches seem to have their pros and cons, I felt that leaving it as it is would also be fine 👌
|
As a general policy, I agree with this change 👍 |
Fixes #996
Before submitting the PR make sure the following are checked:
master(if not - rebase it).Added tests.Updated documentation.Added an entry to theCHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).