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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add contains-value? function #520

Merged
merged 6 commits into from Oct 24, 2022
Merged

Conversation

Chemaclass
Copy link
Member

馃 Background

I think it would be practical if we have a function that checks if a value is inside a collection. Similar as the contains? 馃

馃敄 Changes

  • Add contains-value? function

@Chemaclass Chemaclass added the enhancement New feature or request label Oct 1, 2022
@Chemaclass Chemaclass self-assigned this Oct 1, 2022
@jenshaase
Copy link
Member

array_flip only works with string and integer values. It is not working for something like this:

(contains-value? [:a :b] :a)

Also the current implementation is not working for maps:

(contains-value? {:a 1 :b 2} 1) # => error

I would suggest to solve this problem with existing Phel functions only. Maybe the for loop is an option.

@jenshaase
Copy link
Member

Isn't the find function basically what you are looking for? Don't know how it behaves on maps, but you can combine it with the values function and it should work.

@Chemaclass Chemaclass marked this pull request as draft October 21, 2022 07:56
@Chemaclass
Copy link
Member Author

That's a good idea. Actually changing the function to this helps a lot:

(defn contains-value?
  "Returns true if value is present in the given collection, otherwise returns false."
  [coll val]
  (if (empty? coll)
    false
    (not (nil? (find |(= $ val) coll)))))

And all the tests that I wrote pass, but not these two:

(is (false? (contains-value? (set 1) 2)))
(is (true? (contains-value? (set 1) 1)))

Screenshot 2022-10-21 at 19 46 02

Any idea why are only these two tests failing? 馃
What do you think about the new implementation of the contains-value? using find, @jenshaase?

@jenshaase
Copy link
Member

I would suggest to wrap coll with the values function. This will then also support maps.

(defn contains-value?
  "Returns true if value is present in the given collection, otherwise returns false."
  [coll val]
  (if (empty? coll)
    false
    (not (nil? (find |(= $ val) (values coll))))))

@Chemaclass Chemaclass marked this pull request as ready for review October 22, 2022 08:30
@Chemaclass
Copy link
Member Author

@jenshaase, thanks! Code updated 馃憣馃徎

@jenshaase
Copy link
Member

Could you add tests for maps?

@Chemaclass
Copy link
Member Author

Sure thing, @jenshaase. Done.

@Chemaclass Chemaclass force-pushed the feature/add-contains-value-function branch from ed0bf96 to 5456a8b Compare October 24, 2022 07:38
@Chemaclass Chemaclass merged commit 2ada026 into master Oct 24, 2022
@Chemaclass Chemaclass deleted the feature/add-contains-value-function branch October 24, 2022 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants