FIX: A memoization bug in UserLookup and refactor (PR #13692)

@group_lookup memoization bug was introduced in #13587.

GitHub

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 #values and #map. This doesn’t change much.

Imo using 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?

Used directly:

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 :slight_smile: