ClojureScript

`find` on an associative collection does not return collection key

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: 1.7.145
  • Fix Version/s: 1.9.655
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

Instead find returns the passed in key. This means metadata on the key will appear to be lost. Related to CLJS-1496.

  1. CLJS-1497.patch
    13/Nov/16 10:23 AM
    5 kB
    António Nuno Monteiro
  2. CLJS-1497-2.patch
    21/Dec/16 9:03 AM
    6 kB
    António Nuno Monteiro
  3. CLJS-1497-3.patch
    07/Apr/17 11:45 AM
    6 kB
    António Nuno Monteiro
  4. CLJS-1497-4.patch
    07/Apr/17 11:58 AM
    6 kB
    António Nuno Monteiro

Activity

Hide
Rohit Aggarwal added a comment - - edited

Proposed Plan

IAssociative protocol has a function called -entry-at which has been commented out. This function needs to be implemented which will return the necessary data structure, similar to the way it has been done in Clojure.

An example of its implementation for PersistentArrayMap is:

(-entry-at
 [coll k]
 (let [idx (array-map-index-of coll k)]
   (when-not (neg? idx)
     [(aget arr idx) (aget arr (inc idx))])))

We will need to implement this for all the collections which implement that protocol.

A failing test case:

(deftest test-find-meta-cljs-1497
  (let [k        [1 2 3]
        m        {:my "meta"}
        v        1
        xs       {(with-meta k m) v}
        [k' v']  (find xs k)]
    (is (= k k'))
    (is (= v v'))
    (is (= m (meta k')))))
Show
Rohit Aggarwal added a comment - - edited

Proposed Plan

IAssociative protocol has a function called -entry-at which has been commented out. This function needs to be implemented which will return the necessary data structure, similar to the way it has been done in Clojure. An example of its implementation for PersistentArrayMap is:
(-entry-at
 [coll k]
 (let [idx (array-map-index-of coll k)]
   (when-not (neg? idx)
     [(aget arr idx) (aget arr (inc idx))])))
We will need to implement this for all the collections which implement that protocol. A failing test case:
(deftest test-find-meta-cljs-1497
  (let [k        [1 2 3]
        m        {:my "meta"}
        v        1
        xs       {(with-meta k m) v}
        [k' v']  (find xs k)]
    (is (= k k'))
    (is (= v v'))
    (is (= m (meta k')))))
Hide
António Nuno Monteiro added a comment -

Attached patch with proposed fix and tests.

Show
António Nuno Monteiro added a comment - Attached patch with proposed fix and tests.
Hide
Alex Miller added a comment -

dnolen's comment was lost here in a system migration, but he said: "The proposed patch is a breaking change for people who implement custom collections. We need a new protocol `IEntryAt` or something like this."

Show
Alex Miller added a comment - dnolen's comment was lost here in a system migration, but he said: "The proposed patch is a breaking change for people who implement custom collections. We need a new protocol `IEntryAt` or something like this."
Hide
António Nuno Monteiro added a comment - - edited

Attached an updated patch as per review comments.

Show
António Nuno Monteiro added a comment - - edited Attached an updated patch as per review comments.
Hide
David Nolen added a comment -

Finally getting around to commenting on this patch. We don't want to change an existing protocol. `IFind` should be a new protocol with one method `-find` and we should leave the other protocol alone. That is this patch should be additive only please.

Show
David Nolen added a comment - Finally getting around to commenting on this patch. We don't want to change an existing protocol. `IFind` should be a new protocol with one method `-find` and we should leave the other protocol alone. That is this patch should be additive only please.
Hide
António Nuno Monteiro added a comment -

Attached new patch addressing feedback comments.

Show
António Nuno Monteiro added a comment - Attached new patch addressing feedback comments.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: