Clojure

contains? should throw exception on non-keyed collections

Details

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

Description

The contains? function, given a collection which is not an associative (Map, Set, String, array), returns false instead of throwing an exception.

This is a subject of confusion when people call contains? on sequential collections like lists, and on associative collections which do not implement the Associative interface.

Other predicates, such as even?, throw an exception when passed arguments of an invalid type.

Activity

Hide
Stuart Sierra added a comment -

Patch adds fix, adds some tests, and removes tests reflecting the old behavior.

Show
Stuart Sierra added a comment - Patch adds fix, adds some tests, and removes tests reflecting the old behavior.
Stuart Sierra made changes -
Field Original Value New Value
Patch Code and Test [ 10002 ]
Attachment CLJ-932-0001.patch [ 10921 ]
Rich Hickey made changes -
Fix Version/s Backlog [ 10035 ]
Fix Version/s Release 1.5 [ 10150 ]
Hide
Aaron Bedra added a comment -

This seems to be working properly, but what about vectors?

(contains? [1 2 3] 3)
;-> false
Show
Aaron Bedra added a comment - This seems to be working properly, but what about vectors?
(contains? [1 2 3] 3)
;-> false
Hide
Andy Fingerhut added a comment -

The doc string for contains? covers the vector and Java array case explicitly. I'm not saying that this behavior shouldn't change, but at least it is well documented what it currently does in these cases.

Show
Andy Fingerhut added a comment - The doc string for contains? covers the vector and Java array case explicitly. I'm not saying that this behavior shouldn't change, but at least it is well documented what it currently does in these cases.
Hide
Aaron Bedra added a comment -

Agreed. I just want to make sure that we are still ok with this functionality given that things are changing. Are there others (Stuart) that want to chime in here and make the intentions clear? If this is good then I would consider this screened and ready.

Show
Aaron Bedra added a comment - Agreed. I just want to make sure that we are still ok with this functionality given that things are changing. Are there others (Stuart) that want to chime in here and make the intentions clear? If this is good then I would consider this screened and ready.
Hide
Stuart Sierra added a comment -

Vector is Associative, so supporting contains? is valid even if it does not do what people might expect:

user=> (contains? [:a :b :c] 2)
true
user=> (contains? [:a :b :c] 7)
false

All I'm trying to change here is have contains? throw an exception if the argument is not Associative. The current behavior (returning false) was hiding a bug in my code.

I do not consider this a breaking change. I believe the docstring of contains? leaves room for this interpretation, but Rich will have the final say.

Show
Stuart Sierra added a comment - Vector is Associative, so supporting contains? is valid even if it does not do what people might expect:
user=> (contains? [:a :b :c] 2)
true
user=> (contains? [:a :b :c] 7)
false
All I'm trying to change here is have contains? throw an exception if the argument is not Associative. The current behavior (returning false) was hiding a bug in my code. I do not consider this a breaking change. I believe the docstring of contains? leaves room for this interpretation, but Rich will have the final say.
Hide
Aaron Bedra added a comment -

Perfect. I just wanted to make sure that this was intended.

Show
Aaron Bedra added a comment - Perfect. I just wanted to make sure that this was intended.
Aaron Bedra made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: