Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add index-of #95

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Add index-of #95

merged 1 commit into from
Oct 18, 2024

Conversation

darkleaf
Copy link
Contributor

@darkleaf darkleaf commented Sep 25, 2024

Fixes #54

src/medley/core.cljc Outdated Show resolved Hide resolved
src/medley/core.cljc Outdated Show resolved Hide resolved
src/medley/core.cljc Outdated Show resolved Hide resolved
src/medley/core.cljc Outdated Show resolved Hide resolved
@weavejester
Copy link
Owner

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.

@darkleaf darkleaf force-pushed the index-of branch 4 times, most recently from 6b34885 to b8bf6fa Compare October 11, 2024 15:16
@weavejester
Copy link
Owner

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
Copy link
Owner

Thanks!

@weavejester weavejester merged commit 923d478 into weavejester:master Oct 18, 2024
1 check passed
@NoahTheDuke
Copy link

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

@weavejester
Copy link
Owner

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
Copy link

Sorry for not noticing until it got merged. 😓

@weavejester
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggested addition: index-of
4 participants