janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
125 stars 36 forks source link

added misc/map-keys-flat to map keys without recursing #139

Closed tionis closed 1 year ago

tionis commented 1 year ago

As discussed in https://github.com/janet-lang/spork/pull/136 here's map-keys-flat in it's own PR.

sogaiu commented 1 year ago

Does tabseq work for this kind of situation?

(defn mkf
  [f data]
  (tabseq [[k v] :pairs data]
    (f k) v))

(mkf inc {1 :a 2 :b})
# =>
@{2 :a 3 :b}
tionis commented 1 year ago

It should, but does it bring benefit over the slightly more simplistic version?

sogaiu commented 1 year ago

I think when one is the author of code, the situation is often different from when one is a reader of code one is seeing for the first time. Shortly after expressing code, I think an author probably has more related details still relatively fresh in mind whereas a reader who is unfamiliar with the code has more holes to "fill in" to understand things.

If one is familiar with tabseq (not necessarily a good assumption perhaps), I would guess that the tabseq version is easier to perceive and hence understand:

(defn mkf
  [f data]
  (tabseq [[k v] :pairs data]
    (f k) v))

I would be surprised if it doesn't take somewhat longer (and involve more steps) to process the (edited) loop version:

(defn map-keys-flat
  [f data]
  (def res @{})
  (loop [[k v] :pairs data]
    (put res (f k) v))
  res)

as there is an additional identifier (res) to have in mind, and there are more forms to bring into one's awareness (e.g. the def form, the put form, and the res at the end). I think the additional forms must be coordinated appropriately to achieve an end -- something all of us can get wrong -- so a reader might be thinking about what that coordination might be and whether it was accomplished appropriately. That's absent from tabseq and hence a non-issue.

If one is familiar with both tabseq and loop, it might raise the question of why loop was chosen. Like may be there's something special it's doing that is awkward with tabseq.

Anyway, in isolation, this might not seem like much, but usually these bits sit in a file with other code and in sum they can start to add up especially when investigating issues and one is trying to hold in mind multiple functions and macros.

Don't know whether that's an issue in this case at the moment but as things get added to files that might be different down the line...

(I don't claim to do this kind of thing diligently in my own code -- I don't always attend to it or notice where it might be done, but I suspect I'd benefit a bit if I did it more (^^; )

tionis commented 1 year ago

You make a convincing argument, I updated the code to your suggestion.