@group_lookup memoization bug was introduced in #13587.
I don’t feel like this improves readability that much that we should make the implementation less efficient by doing 2 loops.
Since we’re refactoring this, I’m wondering if we should just make this into a constant. Not sure why we need a method call here.
Note that this will allocate 3 extra arrays as compared to the previous implementation.
I prefer the previous implementation actually since it mutates the existing array instead of allocating new ones.
It’s a public interface now, used by plugins so we’d have to support both for a while, update the plugins and then remove the methods. Not sure if it’s worth it.
Imo the difference in memory characteristics is negligible. (and
#flatten! does create a new array anyway) Readability and consistency takes a priority here (as it does in most cases)
As in the previous comment,
#flatten! does create a new array under the hood, so does
#map. This doesn’t change much.
being/end block and a helper variable to avoid using a functional chain is an unnecessary micro-optimization. The first iterator reduced the number of elements in collection so it’s not like it loops twice over all elements.
It depends on the size of user_ids in this case. If the number of elements in the array is huge, the extra memory is non-negligible.
Is this used by plugins via plugin APIs?
tbh, I don’t mind these being methods (allows e.g. for easier debug tracing by plopping in a
binding.pry) but could go either way.
I understand where you’re coming from but I don’t feel very strongly that this refactor improves readability that much.
Ahh so the ship has already sailed on this. Ignore my comments here then