Skip to content

Conversation

gavlooth
Copy link
Contributor

No description provided.

Copy link
Member

@jeaye jeaye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I have one suggestion and I've looped in Emma for a review as well.

(testing "function sometimes returns []"
(is (= [1 3 5] (mapcat #(if (odd? %) [%] []) (range 1 6)))))
(testing "function sometimes returns nil"
(is (= [2 4] (mapcat #(if (even? %) [%] nil) (range 1 5)))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mapcat has a variadic arity, which we should test by passing many sequences. The most we're doing, in these tests, is two sequences.

@E-A-Griffin
Copy link
Collaborator

Thanks for both of y'all's work! I'll take a look tonight

Copy link
Collaborator

@E-A-Griffin E-A-Griffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Just some small suggestions



(when-var-exists mapcat
(deftest common-cases
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick but why isn't the test name based on the var it's testing?

(mapcat identity 5))))
(testing "non-function first arg"
(is (thrown? #?(:cljs :default :clj Exception :cljr Exception)
(mapcat 42 [1 2]))))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add one more negative test for when the result of mapping isn't a concatable data structure, e.g.

(mapcat identity (range 2))
;; => Exception

@E-A-Griffin
Copy link
Collaborator

@gavlooth if you're busy I don't mind making the changes, just would need write access to push to your fork

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.

4 participants