weavejester / medley

A lightweight library of useful Clojure functions
Eclipse Public License 1.0
870 stars 67 forks source link

Add index-of #95

Closed darkleaf closed 4 weeks ago

darkleaf commented 1 month ago

Fixes https://github.com/weavejester/medley/issues/54

weavejester commented 1 month ago

Thanks! I just thought of two more things, but pretty sure these are the last:

  1. Can you add {:added "1.9.0"} metadata to the index-of function?
  2. Can you change at least one of the tests so the index returned is something other than 1? Just to give us a little more coverage against potential regressions.
weavejester commented 4 weeks ago

Thanks for the updates. Can you wrap the docstring at 80 characters, as per a previous comment? e.g.

(defn index-of
  "Returns the index of the first occurrence of the item in the sequential
  collection coll, or -1 if not found."
  {:added "1.9.0"}
  [^java.util.List coll item]
  (if (nil? coll)
    -1
    (.indexOf coll item)))

For some reason it's wrapped at a much smaller width. Once that's done I'll merge it in and cut a release.

weavejester commented 4 weeks ago

Thanks!

NoahTheDuke commented 4 weeks ago

This doesn't match clojure.string/index-of's behavior (return nil if not found). Is it intentional to have a mismatch?

weavejester commented 4 weeks ago

I assume the idea was to have it match the .indexOf method. However, it's a good point that the Clojure clojure.string/index-of function works differently, as it doesn't have to use primitives and also has nil-punning.

I think you're right though, @NoahTheDuke. I think it makes more sense for index-of to return nil rather than -1. I'll make the necessary changes.

NoahTheDuke commented 4 weeks ago

Sorry for not noticing until it got merged. 😓

weavejester commented 4 weeks ago

You deserve credit for noticing it and catching it before I released!