hsl
hsl copied to clipboard
hhvm
"Fix" Dict\pull
- non-descriptive name based on past internal FB functions
- valuefunc before keyfunc feels strange.
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
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.
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
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
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 !
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.
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.
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. :)
