Fork me on GitHub
#leiningen
<
2019-04-10
>
ikitommi09:04:25

wrote an issue about how leiningen merges maps - can’t remove values from those. Does anyone know/remember if there was a reason for this? or just a bug? https://github.com/technomancy/leiningen/issues/2556

mikerod13:04:40

@ikitommi I think I’ve seen nested meta supporting structures used instead for those cases before so the replace metadata worked.

mikerod13:04:30

But yeah, offhand I don’t know why the behavior is that way regarding nil. I guess sort of makes sense if that’s the merge semantic you were going for in some situations. Merging as combining instead of overwriting.

mikerod13:04:42

And lein mostly does combining merges of values

ikitommi14:04:43

I think it would be a small change to check if the key exists and value is nil, and it should override the value to nil. Like it works with non-collections already. There are at least three copies of meta-merge, would not want to make a fourth copy with just this change.

ikitommi14:04:49

Using it in all projects, really like the way it allows the user be in charge how things are merged.

mikerod15:04:14

@ikitommi I agree it’d be nice to do what you want in some cases. I don’t think it auto handling nil that way makes sense though.

mikerod15:04:05

I think there are cases where you’d not want nil to wipe out other values. Instead “combine” them

mikerod15:04:34

Maybe when the merging values are non-collections it may be ok. But that may be a weird stipulation.

mikerod15:04:19

I often wish meta merging let you use a placeholder value to wrap a value when you needed to add metadata like :replace to something that doesn’t support it

mikerod15:04:37

Like some predefined box type record

mikerod15:04:45

{:a ^:replace #MergeWrapper {:value nil}}

ikitommi15:04:26

My use case is with the #reitit routing library. I use meta-merge to accumulate route data from a route tree into endpoints. It works like a charm in all cases but in the one where I one want’s to set some default in the top-level and override (remove) that default somewhere along the way. The common case:

(def router
  (r/router
    ["/api" {:interceptors [::api]}
     ["/ping" ::ping]
     ["/admin" {:roles #{:admin}}
      ["/users" ::users]
      ["/db" {:interceptors [::db]
              :roles ^:replace #{:db-admin}}]]]))
, get’s expanded (and meta-merged) into:
(r/routes router)
; [["/api/ping" {:interceptors [::api]
;                :name :user/ping}]
;  ["/api/admin/users" {:interceptors [::api]
;                       :roles #{:admin}
;                       :name ::users} nil]
;  ["/api/admin/db" {:interceptors [::api ::db]
;                    :roles #{:db-admin}}]]

ikitommi15:04:27

but adding a content-negotiation is done my setting a :muuntaja key with a instance (non-collection) as a value, preferaby to the root, effecting all routes. With the current meta-merge impl, it’s impossible to unset the value without some marker values like false etc. The thing that meta-merge handles explicit nil differently than the clojure.core/merge makes me think it’s a bug.

ikitommi16:04:32

If this would be fixed in Leiningen, the change would be mirrored to the meta-merge library I’m using (https://github.com/weavejester/meta-merge). Another option is to make a friendly copy and name it somethin like ctrl-merge. There is already a bit-different duct-meta-merge around (https://github.com/duct-framework/core/blob/master/src/duct/core/merge.clj).

mikerod16:04:16

@ikitommi yeah, I understand you have a usage

mikerod16:04:29

but I’m saying the :replace metadata already exists for the concept of “replacing”/overwriting

mikerod16:04:47

the downfall to it is that it only works on things that support metadata - like not on nil right?

ikitommi16:04:59

yes, doesn’t work with nil.

mikerod16:04:04

So what I was saying

mikerod16:04:23

Was it’d be nice if the meta-merge transparently supported a “special wrapper type” that could take metadata for things that cannot have metadata

mikerod16:04:34

Then the same :replace mechanism could be used in any case - as well as :displace and whatever else

mikerod16:04:54

instead of hardcoding how something like nil is handled

ikitommi16:04:43

hmm. I think the fix would be generic if it checked the map entry - if that exists and the value is not a collection, it would override.

mikerod16:04:48

Or maybe it’s all just about nil

mikerod16:04:56

And in that case, maybe top-level map could have metadata idk

ikitommi16:04:16

as it works already with all non-collections, like (meta-merge {:a 1} {:a 2}) ; => {:a 2})

mikerod16:04:28

Yeah, it’s possibly all about nil

mikerod16:04:46

but you want nil to override, not sure all cases would want that - maybe you could justify as that’s just what non-collections do

mikerod16:04:24

I do wonder if things like lein take advantage of the current nil behavior though

mikerod16:04:56

There probably could be subtle issues changing it.

ikitommi16:04:58

I was trying to find an example of that usage, but couldn’t find any..

mikerod16:04:18

I think some cases may be hard to see

mikerod16:04:27

Profiles merging and having default keys added etc

ikitommi16:04:28

yeah, tons of private repos etc.

mikerod16:04:49

I don’t think it’s that uncommon for people to put :x nil in a map instead of not adding :x at all

mikerod16:04:06

in clj I find, you often have to go out of your way to not put the key in the map - it’s uglier

mikerod16:04:24

so in those cases, if that’s automatically happening, and it gets in front of a meta-merge - with your change, it’ll start wiping out other values instead of being ignored

ikitommi16:04:57

so, too risky to change as something could break?

mikerod16:04:31

What I mean here: > in clj I find, you often have to go out of your way to not put the key in the map - it’s uglier (assoc m :x <don't know if I have this>) is commonly done, when really (cond-> m (some? <don't know if I have this>) (assoc :x <don't know if I have this>)) would be more appropriate

mikerod16:04:20

> so, too risky to change as something could break? I don’t really know. I’m just thinking it’d have to be looked into.

ikitommi16:04:47

ok, thanks for giving it a thought. I understand if that can’t be changed, kinda fundamental and could break something - but there is now the issue of this.

Alex Miller (Clojure team)16:04:19

just another option, add meta-merge2 and users can migrate as needed

Alex Miller (Clojure team)16:04:25

accretion, not breakage

mikerod16:04:23

@alexmiller Yeah, definitely not a fan of using the same name for breaking things. I’m onboard with that concept. I’m not sure how the separate projects are kept in sync or whatever though - meta-merge vs lein.

mikerod16:04:05

I think it was already mentioned to copy/rename > If this would be fixed in Leiningen, the change would be mirrored to the meta-merge library I’m using (https://github.com/weavejester/meta-merge). Another option is to make a friendly copy and name it somethin like ctrl-merge. So that’s probably the meta-merge2 concept.

ikitommi20:04:26

If Leiningen doesn't need the nil thing, them the version 2 should be issue of the meta-merge library, right?

ikitommi20:04:55

If there should be a bug in the merge, effecting both meta-merge1 & 2, should the fix be applied in both? Any guidelines when to stop maintaining old versions (like soon: spec1)?

ikitommi20:04:04

also, if a libraryX updates from using mm1 to mm2, should that lib also change the namespaces so that "no-one breaks"?

ikitommi20:04:11

would be nice to have some written guidelines for preferred clojure accretion with libs, ping @alexmiller

Alex Miller (Clojure team)20:04:53

There are not always simple answers to all of these questions. Rich’s Spec-ulation talk is still the best rendering of the problems

Alex Miller (Clojure team)20:04:33

In general, turn breakage into accretion

Alex Miller (Clojure team)20:04:21

Fixation should generally be considered non-breaking as a matter of definition

Alex Miller (Clojure team)20:04:49

But sometimes that is subtle if users rely on the “broken” behavior