Clojure

The update function: like update-in, for first level

Details

  • Type: Enhancement Enhancement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.6
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

update-in is useful for updating nested structures. Very often we just want to update one level, so an update function optimised for this use case is useful.

It operates identically to update-in with a key path of length one so these are the same:

(update-in m [k] f args...)
(update m k f args...)

Patch: CLJ-1251.patch

Screened by: Alex Miller

  1. update.patch
    03/Sep/13 9:01 AM
    2 kB
    Michael O. Church
  2. CLJ-1251.patch
    23/May/14 12:16 PM
    4 kB
    Ambrose Bonnaire-Sergeant

Activity

Hide
Alex Miller added a comment -

I like this - kind of halfway between assoc and update-in.

Show
Alex Miller added a comment - I like this - kind of halfway between assoc and update-in.
Hide
Michael O. Church added a comment -

It's very useful. I assumed that its non-inclusion was for a reason (hence was hesitant to submit the patch) but it comes in handy a lot. One project I'd like to do with some free time is a library for turn-based strategy games, which use update frequently to express game-state changes.

The downside of this change is that 'update is probably a defined function in a good number of modules written by other people. IMO the strongest reason not to include it is that it's such a common name; but the benefits (in my view) outweigh the downsides.

Show
Michael O. Church added a comment - It's very useful. I assumed that its non-inclusion was for a reason (hence was hesitant to submit the patch) but it comes in handy a lot. One project I'd like to do with some free time is a library for turn-based strategy games, which use update frequently to express game-state changes. The downside of this change is that 'update is probably a defined function in a good number of modules written by other people. IMO the strongest reason not to include it is that it's such a common name; but the benefits (in my view) outweigh the downsides.
Hide
Andy Fingerhut added a comment -

Patch update.patch dated Sep 3 2013 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.

Show
Andy Fingerhut added a comment - Patch update.patch dated Sep 3 2013 no longer applies cleanly to latest Clojure master as of Feb 14 2014. It did on Feb 7 2014. I haven't checked in detail, but this is probably simply due to some tests recently added to a test file that require updating some diff context lines.
Hide
Ambrose Bonnaire-Sergeant added a comment -

The vararg validation should be done in the same way as `assoc`.

Show
Ambrose Bonnaire-Sergeant added a comment - The vararg validation should be done in the same way as `assoc`.
Hide
Alan Malloy added a comment - - edited

The most obvious reason, to me, that clojure.core/update doesn't exist already is that it's not clear what it should do when given more than 3 arguments. Consider, for example, (update m a b c d). What does this do? There are at least three reasonable interpretations: (update-in m [a] b c d), passing c and d as extra args to the function b; (-> m (update-in [a] b) (update-in [c] d)), treating the args as alternating key/function pairs; (reduce (fn [m k] (update-in m [k] a)) m [b c d]), treating a as a function to apply to each of b, c, and d.

Any of these are plausible meanings for the vague name "update", and there's no obvious behavior to choose, whereas there's only one reasonable way for assoc and assoc-in to behave. If one of them were chosen, it would be a little bit nontrivial to read code using it, at least until it became so well-known that everyone thinks it's obvious. I don't have anything against this function that Michael Church has written, or including it in core, but I don't like naming it update, as if it were the only possible dual to update-in.

Show
Alan Malloy added a comment - - edited The most obvious reason, to me, that clojure.core/update doesn't exist already is that it's not clear what it should do when given more than 3 arguments. Consider, for example, (update m a b c d). What does this do? There are at least three reasonable interpretations: (update-in m [a] b c d), passing c and d as extra args to the function b; (-> m (update-in [a] b) (update-in [c] d)), treating the args as alternating key/function pairs; (reduce (fn [m k] (update-in m [k] a)) m [b c d]), treating a as a function to apply to each of b, c, and d. Any of these are plausible meanings for the vague name "update", and there's no obvious behavior to choose, whereas there's only one reasonable way for assoc and assoc-in to behave. If one of them were chosen, it would be a little bit nontrivial to read code using it, at least until it became so well-known that everyone thinks it's obvious. I don't have anything against this function that Michael Church has written, or including it in core, but I don't like naming it update, as if it were the only possible dual to update-in.
Hide
Kyle Kingsbury added a comment - - edited

I'd like to second Alan Malloy's concern; I've defined (update m k f arg1 arg2) in most of my Clojure work to be "change the value for this key to be (f current-value arg1 arg2 ...)"; this is consistent with swap!, update-in, etc., and is in my experience the most common need for update. It also composes well with swap! and other higher-order friends. I suggest we use that variant instead, and rely on assoc or -> threading when updating multiple fields.

Show
Kyle Kingsbury added a comment - - edited I'd like to second Alan Malloy's concern; I've defined (update m k f arg1 arg2) in most of my Clojure work to be "change the value for this key to be (f current-value arg1 arg2 ...)"; this is consistent with swap!, update-in, etc., and is in my experience the most common need for update. It also composes well with swap! and other higher-order friends. I suggest we use that variant instead, and rely on assoc or -> threading when updating multiple fields.
Hide
Michael O. Church added a comment -

I agree with Kyle and Alan. There are several interpretations of how update should behave and while it's not clear which one is "correct", Kyle's is most consistent with the rest of the language and therefore probably more right than the one I started with.

The issue I see with including an "update" function is that it will break code for others who've defined it for themselves. Kyle's interpretation is more consistent with the rest of Clojure and will probably involve the least breakage. I'd be happy using his version, and renaming mine to something else.

Show
Michael O. Church added a comment - I agree with Kyle and Alan. There are several interpretations of how update should behave and while it's not clear which one is "correct", Kyle's is most consistent with the rest of the language and therefore probably more right than the one I started with. The issue I see with including an "update" function is that it will break code for others who've defined it for themselves. Kyle's interpretation is more consistent with the rest of Clojure and will probably involve the least breakage. I'd be happy using his version, and renaming mine to something else.
Hide
Rich Hickey added a comment -

I am in favor, and it should work like everything else: (update m k f args...)

Show
Rich Hickey added a comment - I am in favor, and it should work like everything else: (update m k f args...)
Hide
Ambrose Bonnaire-Sergeant added a comment -

I'm working on a new patch.

Show
Ambrose Bonnaire-Sergeant added a comment - I'm working on a new patch.
Hide
Ambrose Bonnaire-Sergeant added a comment -

update-like-update-in.patch is the new patch as Rich requests.

Show
Ambrose Bonnaire-Sergeant added a comment - update-like-update-in.patch is the new patch as Rich requests.
Hide
Alex Miller added a comment -

Ambrose, I think the example in the description no longer follows the (update m k f args...) form right? Can you update?

Show
Alex Miller added a comment - Ambrose, I think the example in the description no longer follows the (update m k f args...) form right? Can you update?
Hide
Ambrose Bonnaire-Sergeant added a comment -

Alex, I'm not sure what you're referencing?

Show
Ambrose Bonnaire-Sergeant added a comment - Alex, I'm not sure what you're referencing?
Hide
Ambrose Bonnaire-Sergeant added a comment -

If you mean the docstring, I did try and update it for update by copying update-in and change and plural keys to singular.

Show
Ambrose Bonnaire-Sergeant added a comment - If you mean the docstring, I did try and update it for update by copying update-in and change and plural keys to singular.
Hide
Alex Miller added a comment -

I mean the description for this ticket needs to be updated to reflect what we are currently considering.

Show
Alex Miller added a comment - I mean the description for this ticket needs to be updated to reflect what we are currently considering.
Hide
Alex Miller added a comment -

In the patch, the docstring has "If the key does not exist, a hash-map will be created." which is not applicable in update right? I think it would be more accurate to say that the fn will be invoked on nil.

This line occurs twice in the tests:

{:a [1 2]}   (update {:a [1]} :a conj 2)

There is no test for what happens when the key is absent. For example:

(update {:a 1} :b str)
=> {:b "", :a 1}
Show
Alex Miller added a comment - In the patch, the docstring has "If the key does not exist, a hash-map will be created." which is not applicable in update right? I think it would be more accurate to say that the fn will be invoked on nil. This line occurs twice in the tests:
{:a [1 2]}   (update {:a [1]} :a conj 2)
There is no test for what happens when the key is absent. For example:
(update {:a 1} :b str)
=> {:b "", :a 1}
Hide
Ambrose Bonnaire-Sergeant added a comment -

I removed the mention of creating hash-maps, and replaced it with the explicit behaviour of passing `nil` for missing keys.

FWIW I proposed a similar wording in the patch for http://dev.clojure.org/jira/browse/CLJ-373

Added a test for missing key. Removed the duplicate test.

Show
Ambrose Bonnaire-Sergeant added a comment - I removed the mention of creating hash-maps, and replaced it with the explicit behaviour of passing `nil` for missing keys. FWIW I proposed a similar wording in the patch for http://dev.clojure.org/jira/browse/CLJ-373 Added a test for missing key. Removed the duplicate test.
Hide
Gary Fredericks added a comment -

Is it worth unrolling several arities for the sake of premature optimization? e.g., https://github.com/Prismatic/plumbing/blob/master/src/plumbing/core.clj#L33-41

Show
Gary Fredericks added a comment - Is it worth unrolling several arities for the sake of premature optimization? e.g., https://github.com/Prismatic/plumbing/blob/master/src/plumbing/core.clj#L33-41
Hide
Alex Miller added a comment -

I think that's probably worth doing - who can update the patch with multiple arities?

Show
Alex Miller added a comment - I think that's probably worth doing - who can update the patch with multiple arities?
Hide
Alex Miller added a comment -

Ambrose, can you (or anyone else really) update the patch to unroll small arities?

Show
Alex Miller added a comment - Ambrose, can you (or anyone else really) update the patch to unroll small arities?
Hide
Ambrose Bonnaire-Sergeant added a comment -

Yes will do now.

Show
Ambrose Bonnaire-Sergeant added a comment - Yes will do now.
Hide
Ambrose Bonnaire-Sergeant added a comment -

Add multiple arities + tests (CLJ-1251.patch)

Show
Ambrose Bonnaire-Sergeant added a comment - Add multiple arities + tests (CLJ-1251.patch)

People

Vote (3)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: