"Fix" Dict\pull

[fredemmott]

• non-descriptive name based on past internal FB functions
• valuefunc before keyfunc feels strange.

[savil]

Naming is hard!
Also need to consider Dict\map, Dict\map_keys and Dict\map_with_key.

"pull" is a fancy mapping transform, so can we call it that?

suggestion #1 (most consistent with the current naming. Focuses on what the outputs are, and uses "with_keys" for the second case):
Dict\pull -> Dict\map_values_and_keys
Dict\pull -> Dict\map_values_and_keys_with_keys // oof, so verbose :-(

suggestions #2 (introduce "from"):

• Dict\pull -> Dict\map_from_values
• Dict\pull_with_key -> Dict\map_from_values_and_keys

suggestion #3 (just drop the map, but this may not fit the naming guidelines):

• Dict\pull -> Dict\from_values
• Dict\pull_with_key -> Dict\from_values_and_keys

[fredemmott]

Personally I'd prefer map_keys_and_values as it's dict<tk, tv> not dict<tv, tk> - @kmeht is worried about verbosity of this kind of thing though.

[fredemmott]

Strongly dislike 2 and 3 though: 2 the name doesn't really give me any expectation of what they'd do

3 seems misleading: I'd expect Dict\from_values_and_keys to take Traversable<Tk> and Traversable<Tv> of the same length

[savil]

re: #1
yeah, no strong opinions. I wrote down map_values_and_keys because that is the order of the parameters i.e. value_func is first and then key_func.

Verbosity is definitely a challenge in these. "pull" is more memorable, once you get past the wtf does that do stage :-)

re: #2 and #3: fair points

[savil]

because that is the order of the parameters i.e. value_func is first and then key_func

ah, I now see that in your initial issue-description, you'd mentioned possibly swapping the parameter order. That would reconcile this with your preference of map_keys_and_values.

So, remaining concern is verbosity. Verbosity is more friendly to beginners, and experienced hack people could use autocomplete in their editor. Left to you and @kmeht !

[aosq]

HSL already has Dict\group_by, Dict\pull could becomeDict\index_by? Coming over from ActiveRecord, Enumerable#index_by was easy to remember because of its similarities to Enumerable#group_by.

[fredemmott]

The primary usage of Dict\pull is when using functions for both keys and values; I think for the index_by use case you want Dict\map_keys()?

I'm actually a little tempted to remove it entirely, in favor of mapping to a tuple, then using Dict\from_entries()" (a.k.a. unzip), but it's common enough I suspect people will just re-add it to their local codebases.

[admdikramr]

IMO we should leave this alone. You pull one dict out of another. Maybe it's like turning a sweater inside out, maybe it's like pulling a rabbit out of a hat, but the current name is descriptive if not obvious. We have a huge problem in the HSL wherein many many of our method names are not obvious and there's no easy way to search them or find them -- let's not make this worse.

Dict\pull is also a multipurpose amalgam method. It's very helpful and useful but it just wraps together Dict\map and Dict\map_keys in a way that gives you access to the old values for both (and the old keys for both in Dict\pull_with_key) when generating the new values for both. This makes it really hard to name with our current naming scheme which is already overly verbose / descriptive and thus easy to forget, when we have no reasonable way to actually search for what we mean. So the cost of changing from a short, snappy, easy-to-remember method name (pull) to anything longer is probably worse than doing nothing.

I propose we close this as "fixed" and say that we fixed our intuitions that "pull" is a bad name. :)