weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
865 stars 66 forks source link

Retain metadata for polyadic assoc-some #93

Open tomdl89 opened 1 month ago

tomdl89 commented 1 month ago

Fixes issue raised in #91

tomdl89 commented 1 month ago

On a related note, reduce-map could benefit from this too: https://github.com/tomdl89/medley/commit/8fd5920491cf15deb55d30baddab69bbf908ee64 I'm not convinced https://github.com/weavejester/medley/issues/13 reached the right conclusion btw. Comparing to map filter remove etc doesn't seem to make sense, as those functions produce a lazy-seq from a potentially different coll, whereas map-vals filter-vals remove-vals etc always take a map and return a map. Indeed, update-vals in latest clojure versions (which is doing the same thing as map-vals) does retain metadata.

weavejester commented 1 month ago

Thanks for the commit, and apologies for the delay in reviewing it. Can you add a simple test just to prevent any regressions in future.

With regard to #13, I was originally basing my decision on mapv, which takes a vector and returns a vector, but discards the metadata. It's interesting that with update-vals they've decided to go the other way.

I'm willing to keep metadata for maps if that's the precedent set by clojure.core. I'm just concerned about keeping behavior consistent.

NoahTheDuke commented 1 month ago

I think the reasoning is that functions that modify an object retain metadata, and sequence functions don't. update-vals follows after assoc and update by retaining metadata, whereas mapv follows after map which does not. I would expect an assoc-like function to retain metadata.

weavejester commented 1 month ago

Then perhaps the rule is that functions with the collection as the first argument retain metadata, and otherwise it doesn't. Hence update-vals retaining metadata, and mapv not doing so.

weavejester commented 3 days ago

Looks like this PR is still waiting on a test. Would you be able to write one @tomdl89, or do you not have time? If needs be, I can write one.