search

weavejester / meta-merge

A standalone implementation of Leiningen's meta-merge function
104 stars 5 forks source link

nil-values in maps merge differently than with clojure.core/merge #11

Closed ikitommi closed 1 year ago

ikitommi commented 6 years ago

Should nil values be merged like normal merge does, e.g. last value wins?

(merge {:a 1} {:a nil})
; => {:a nil}

(meta-merge.core/meta-merge {:a 1} {:a nil})
; => {:a 1}
weavejester commented 6 years ago

meta-merge mimics the functionality in Leiningen, so it does whatever Leiningen's merge logic does.

ikitommi commented 6 years ago

Ok. I still think this is not correct. What do you think? Instead of having forks/copies that change/add the original Leiningen functionality, could there be a 2.0.0 with the duct-addons and fixes for things like this?

weavejester commented 6 years ago

Are you asking this to be changed in Duct or changed in meta-merge?

ikitommi commented 6 years ago

in meta-merge. Would be good if there wasn't 3 (meta-merge, duct and reitit) bit different meta-merge syntaxes out there.

weavejester commented 6 years ago

Changing the behavior of meta-merge would mean that there would be four different meta-merge syntaxes, since meta-merge the library would diverge from the implementation in Leiningen.

There seem to be explicit cases for nil in the code, so I'd like to know why they were put in place before changing them. There's also the problem that this behavior change would make meta-merge incompatible with Leiningen's profile merging.

It might be a better idea to try to get this behavior changed in Leiningen first. Or at least ask why profiles are merged in such a manner.

ikitommi commented 5 years ago

Had a discussion about this in the #leiningen slack. Changing that could break anything, so not likely going to happen. I guess same applies here, so I guess the alternatives are:

1) create a meta-merge 2.0 with this change (and maybe other new stuff from duct) 2) I inline a ctrl-merge impl into reitit with this change (and maybe others from duct)

I would be much happier with the 1 option. What do you think?

weavejester commented 5 years ago

I'd need to consider the consequences of changing meta-merge's behaviour in this case. I don't think I'll have time in the near future to do that and create a meta-merge 2.0. It might happen, but not within the next few months.

ikitommi commented 5 years ago

Ok. I'll re-visit how mandatory & far reaching this is in reitit (would require a major version bump anyway) - if it is, will go with 2 and switch back to meta-merge2 when/if it gets implemented.

Big thanks for making meta-merge a separate library, using it in all the projects now, a fundamental library and superior to the various deep-merge impls out there.

ikitommi commented 1 year ago

I forked meta-merge to add the missing features I need. I'll ping you @weavejester when it's done, happy to PR changes back here if you think it could it could be part of meta-merge2.

https://github.com/metosin/ctrl-merge/blob/master/CHANGELOG.md

weavejester commented 1 year ago

I could create a meta-merge2.core namespace to implement these breaking changes, but I think it makes more sense to keep meta-merge as it is and instead develop a new library instead. This means meta-merge maintains backward compatibility, but new libraries are free to extend and improve upon meta-merge.

ikitommi commented 1 year ago

That makes sense, thanks. Closing this.