Clojure

Missing dissoc-in

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.4
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Triaged

Description

There is no clojure.core/dissoc-in although there is an assoc-in.
It is correct that dissoc-in can be build with update-in and dissoc but this is an argument against assoc-in as well.
When a shortcut for assoc-in is provided, there should also be one for dissoc-in for consistency reasons.
Implementation is analogical to assoc-in.

Activity

Hide
Andy Fingerhut added a comment -

Patch clj-1063-add-dissoc-in-patch-v2.txt dated Sep 13 2012 supersedes 001-dissoc-in.diff dated Sep 7 2012. It fixes a typo (missing final " in doc string), and adds a test case for the new function.

Show
Andy Fingerhut added a comment - Patch clj-1063-add-dissoc-in-patch-v2.txt dated Sep 13 2012 supersedes 001-dissoc-in.diff dated Sep 7 2012. It fixes a typo (missing final " in doc string), and adds a test case for the new function.
Hide
Nicola Mometto added a comment -

Thanks for the fix Andy

Show
Nicola Mometto added a comment - Thanks for the fix Andy
Hide
Andy Fingerhut added a comment -

This proposed dissoc-in should be compared with the one in clojure.core.incubator which I just happened across. I see they look different, but haven't examined to see if there are any behavior differences.

https://github.com/clojure/core.incubator/blob/master/src/main/clojure/clojure/core/incubator.clj

Show
Andy Fingerhut added a comment - This proposed dissoc-in should be compared with the one in clojure.core.incubator which I just happened across. I see they look different, but haven't examined to see if there are any behavior differences. https://github.com/clojure/core.incubator/blob/master/src/main/clojure/clojure/core/incubator.clj
Hide
Nicola Mometto added a comment - - edited

dissoc-in in clojure.core.incubator recursively removes empty maps

user=> (clojure.core.incubator/dissoc-in {:a {:b {:c 1}}} [:a :b :c])
{}

while the one in this patch doesn't (as I would expect)

user=> (dissoc-in {:a {:b {:c 1}}} [:a :b :c])
{:a {:b {}}}

Show
Nicola Mometto added a comment - - edited dissoc-in in clojure.core.incubator recursively removes empty maps user=> (clojure.core.incubator/dissoc-in {:a {:b {:c 1}}} [:a :b :c]) {} while the one in this patch doesn't (as I would expect) user=> (dissoc-in {:a {:b {:c 1}}} [:a :b :c]) {:a {:b {}}}
Hide
Stuart Halloway added a comment -

Please do this work in the incubator.

Show
Stuart Halloway added a comment - Please do this work in the incubator.
Hide
Andrew Rosa added a comment -

Keeping the empty paths is really the most expected thing to do? I say, since both assoc-in/update-in create paths along the way this behavior looks to be the real "dual".

What kind of bad things could happen that nil punning does not deal well with it? Those issues would be too much obscure for dissoc-in user's point of view?

Show
Andrew Rosa added a comment - Keeping the empty paths is really the most expected thing to do? I say, since both assoc-in/update-in create paths along the way this behavior looks to be the real "dual". What kind of bad things could happen that nil punning does not deal well with it? Those issues would be too much obscure for dissoc-in user's point of view?
Hide
Alex Miller added a comment -

Tests are much too light in the patch. Should test multiple levels, records, vectors, etc.

Show
Alex Miller added a comment - Tests are much too light in the patch. Should test multiple levels, records, vectors, etc.
Hide
Daniel Compton added a comment - - edited

Testing "clj-1063-add-dissoc-in-patch-v2.txt"

(dissoc-in {} [:a :b :c])
=> {:a {:b nil}}

this behaviour is pretty unexpected. The incubator version doesn't have this problem, though it does remove empty maps.

I'm working on tests (and possibly a new dissoc-in) for this patch, but I'm not clear yet whether empty paths should be kept or removed? I don't mind either way, though I tend to agree with Andrew Rosa that keeping them is more symmetrical.

Show
Daniel Compton added a comment - - edited Testing "clj-1063-add-dissoc-in-patch-v2.txt"
(dissoc-in {} [:a :b :c])
=> {:a {:b nil}}
this behaviour is pretty unexpected. The incubator version doesn't have this problem, though it does remove empty maps. I'm working on tests (and possibly a new dissoc-in) for this patch, but I'm not clear yet whether empty paths should be kept or removed? I don't mind either way, though I tend to agree with Andrew Rosa that keeping them is more symmetrical.
Hide
Daniel Compton added a comment -

@alex, do you want empty paths to be kept or removed? Either way is acceptable, though on balance I slightly prefer leaving them. I can work up tests for this, once I know which direction you want to go in.

Show
Daniel Compton added a comment - @alex, do you want empty paths to be kept or removed? Either way is acceptable, though on balance I slightly prefer leaving them. I can work up tests for this, once I know which direction you want to go in.
Hide
Alex Miller added a comment -

What are the tradeoffs of one vs the other?

In places where people are using dissoc-in stand-ins (update-in with dissoc etc) what behavior are they expecting?

Show
Alex Miller added a comment - What are the tradeoffs of one vs the other? In places where people are using dissoc-in stand-ins (update-in with dissoc etc) what behavior are they expecting?
Hide
Michał Marczyk added a comment -

If we only wanted to handle maps nested in maps …, I'd probably prefer removing empty subpaths.

However, if we want to admit vectors along the way, then leaving empty paths in place makes for pretty clear semantics in that case:

(dissoc-in [{:foo 1}] [0 :foo])
;= [{}]

In contrast, removing empty paths would seem to lead to problems that would have to be resolved either by limiting the path-removing behaviour to maps or throwing when a subpath nested in a vector becomes entry. Either approach seems suboptimal.

Show
Michał Marczyk added a comment - If we only wanted to handle maps nested in maps …, I'd probably prefer removing empty subpaths. However, if we want to admit vectors along the way, then leaving empty paths in place makes for pretty clear semantics in that case:
(dissoc-in [{:foo 1}] [0 :foo])
;= [{}]
In contrast, removing empty paths would seem to lead to problems that would have to be resolved either by limiting the path-removing behaviour to maps or throwing when a subpath nested in a vector becomes entry. Either approach seems suboptimal.
Hide
Michał Marczyk added a comment -

Also we could technically accept a flag to decide whether empty paths should be removed and let the user ensure that they only pass true if there are no vectors in the way. I'm not sure I like this approach very much, though, because it seems to me that the more natural purpose for extra arguments is to provide additional paths to dissoc for matching variadic dissoc and dissoc-in semantics (cf. CLJ-1771).

Show
Michał Marczyk added a comment - Also we could technically accept a flag to decide whether empty paths should be removed and let the user ensure that they only pass true if there are no vectors in the way. I'm not sure I like this approach very much, though, because it seems to me that the more natural purpose for extra arguments is to provide additional paths to dissoc for matching variadic dissoc and dissoc-in semantics (cf. CLJ-1771).
Hide
Alex Miller added a comment -

We must consider vectors or associative data structures other than maps. I also would prefer not to have a flag and to instead leave it open for variadic to remove multiple.

With an equivalent like:

(update-in [{:foo 1}] [0] dissoc :foo)

we would be left with [{}] so the existing way to do this does leave empty collections.

I also looked at some of the existing dissoc-in implementations that already exist - core.incubator, taoensso.encore, clj-http, medley, useful, plumbing, etc. Most work with a single key-path and leave empty collections - these seem largely to derive from the incubator version. The encore one works with multiple paths by separating the key path from the final leaf keys to dissoc (variadic). The medley, useful, and plumbing ones are map-only and remove empty maps.

I can't imagine that we would make a map-specific version of this in core (since all the other -in functions are generic) and thus I conclude that we would need a dissoc-in that left empty collections. Given that, I'm not sure whether dissoc-in provides enough value over update-in + dissoc to be worth adding to core at this point, especially since it exists in a variety of utility libraries (in many cases with differing semantics). We are then potentially breaking (or at least inconveniencing) many users of existing dissoc-in impls. I'm going to leave this open for comments, but I'm of a mind to decline this enhancement at this point.

Show
Alex Miller added a comment - We must consider vectors or associative data structures other than maps. I also would prefer not to have a flag and to instead leave it open for variadic to remove multiple. With an equivalent like:
(update-in [{:foo 1}] [0] dissoc :foo)
we would be left with [{}] so the existing way to do this does leave empty collections. I also looked at some of the existing dissoc-in implementations that already exist - core.incubator, taoensso.encore, clj-http, medley, useful, plumbing, etc. Most work with a single key-path and leave empty collections - these seem largely to derive from the incubator version. The encore one works with multiple paths by separating the key path from the final leaf keys to dissoc (variadic). The medley, useful, and plumbing ones are map-only and remove empty maps. I can't imagine that we would make a map-specific version of this in core (since all the other -in functions are generic) and thus I conclude that we would need a dissoc-in that left empty collections. Given that, I'm not sure whether dissoc-in provides enough value over update-in + dissoc to be worth adding to core at this point, especially since it exists in a variety of utility libraries (in many cases with differing semantics). We are then potentially breaking (or at least inconveniencing) many users of existing dissoc-in impls. I'm going to leave this open for comments, but I'm of a mind to decline this enhancement at this point.
Hide
Nicola Mometto added a comment - - edited

The same argument could've been made for the inclusion of `update`. The fact that both `update` and `dissoc-in` can be found in several util libraries should indicate that people are not ok with just using `update-in` + `dissoc`.

Sure adding it to core is going to cause warnings when using some libraries (it's not going to break anything, the AOT bugs that caused breakages with `update` have been fixed) but I don't think that's really a big deal; new versions of those libraries will get released quickly and even if that's not the case a warning is not the end of the world.

Show
Nicola Mometto added a comment - - edited The same argument could've been made for the inclusion of `update`. The fact that both `update` and `dissoc-in` can be found in several util libraries should indicate that people are not ok with just using `update-in` + `dissoc`. Sure adding it to core is going to cause warnings when using some libraries (it's not going to break anything, the AOT bugs that caused breakages with `update` have been fixed) but I don't think that's really a big deal; new versions of those libraries will get released quickly and even if that's not the case a warning is not the end of the world.
Hide
Alex Miller added a comment -

True, although I'd say the factors balance differently in the update case. With update there fewer implementations and they matched what we were proposing to add. So, they created the potential for the existing ones to be replaced with a core function.

I think the common utility library impls (flatland/useful/plumbing) are actually covering a need that is different - update-in + dissoc + path removal on maps only. So if the goal is to cover a common usage, then we would need to have it work only on maps and remove empty paths left behind. However, I think that is somewhat at odds with the other core -in functions. I guess maybe there is a middle ground to do path removal but make it work beyond maps - that would satisfy the needs indicated by the utility libs but also be generic like the other -in functions. And if you needed to leave the empty colls, you could do what you do now - update-in+dissoc.

Show
Alex Miller added a comment - True, although I'd say the factors balance differently in the update case. With update there fewer implementations and they matched what we were proposing to add. So, they created the potential for the existing ones to be replaced with a core function. I think the common utility library impls (flatland/useful/plumbing) are actually covering a need that is different - update-in + dissoc + path removal on maps only. So if the goal is to cover a common usage, then we would need to have it work only on maps and remove empty paths left behind. However, I think that is somewhat at odds with the other core -in functions. I guess maybe there is a middle ground to do path removal but make it work beyond maps - that would satisfy the needs indicated by the utility libs but also be generic like the other -in functions. And if you needed to leave the empty colls, you could do what you do now - update-in+dissoc.

People

Vote (11)
Watch (7)

Dates

  • Created:
    Updated: