search

janet-lang / spork

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

fixed misc/map-keys handling of array values #136

Closed tionis closed 1 year ago

tionis commented 1 year ago

misc/map-keys was broken for cases like (misc/map-keys |(string $0) {:test [1 2 3]}) in which it produced @{2 3 "test" 1} or even failed with inputs like {:test [1 2 3 4]}. This should fix that by ensuring values are always left as is. I also disabled the default recursion as I think that is quite an unexpected behavior, but I left a flag to enable recursion. This changes the function signature of misc/map-keys, but I think no one uses this function, as anyone having a value with an array in it would discover this bug pretty fast. As this was merged from pepe's marble into spork: have a ping: @pepe

pepe commented 1 year ago

Thank you for the catch.

But I disagree that changing the function signature in the fix PR is OK. I use this function extensively and see your use case as quite a niche :-).

Also, another commit with change is totally unrelated to the topic.

tionis commented 1 year ago

I removed the unrelated commit.

tionis commented 1 year ago

Also, I disagree with you on what would be expected default behavior. But as I judged wrongly and there are in fact users of the old function I kept it as is (with the fix applied) and added a second one (map-keys-flat) that does not recurse.

pepe commented 1 year ago

Anyway, today I would write something more like:

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

Sorry, for the one-liner, just copied it from the repl.

tionis commented 1 year ago

Anyway, today I would write something more like:

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

Sorry, for the one-liner, just copied it from the repl. This does more simplistic, I'll merge this instead.

I'll also split map-keys-flat into a separate PR.

tionis commented 1 year ago

I was not saying that some behavior is expected or not. Sorry if it seemed like this. I just said that I am using the function as is a lot. And the recursivity was mentioned in the docstring :-).

Yeah, I kind of missed that in the docstring (or maybe I didn't read it all?) and was a bit surprised when I first used it.