vickychijwani / closer.js

:hammer_and_wrench: Clojure parser and core library in JavaScript, compatible with the Mozilla Parser API.
https://vickychijwani.github.io/closer.js
MIT License
83 stars 8 forks source link

Clojure Functions added #11

Closed koustuvsinha closed 9 years ago

koustuvsinha commented 9 years ago

Following functions have been added along with tests :

Please review. More functions will be added in this PR as per #10

vickychijwani commented 9 years ago

Looks great, thanks! Will go over the code shortly to give feedback.

vickychijwani commented 9 years ago

I've added some comments on the code for distinct. Let me know if you run into issues.

koustuvsinha commented 9 years ago

Made the required changes!

koustuvsinha commented 9 years ago

Dont merge this PR yet, want to implement a few more functionalities as well ! Need a pointer on how to implement frequencies though. Should I use the map reduce way to count the frequencies of elements or is there any other way to do so?

vickychijwani commented 9 years ago

Here's some info to help you choose the right assertion for collections. Collections can be vectors, lists, seqs, JS arrays, strings, sets, or maps.

Hope this helps!

vickychijwani commented 9 years ago

Map-reduce should work fine for frequencies. You could also use core.clj_$__$GT_js(core.group_$_by(...)) followed by lodash's mapValues ;)

koustuvsinha commented 9 years ago

Got the solution by two ways eventually :

Both the solutions arrive to same conclusion. However have an issue :

vickychijwani commented 9 years ago

Hmm, you're right, that is a problem. I hadn't thought of that. Also the calls to js->clj and vice versa will surely affect performance. I think we can avoid the issue altogether by working purely using Clojure functions:

core.into(core.hash_$_map(),
  core.map(
    ((kv) -> core.vector(core.key(kv), core.count(core.val(kv)))),
    core.group_$_by(core.identity, coll))
koustuvsinha commented 9 years ago

ok, will try that and post the results soon

koustuvsinha commented 9 years ago

yes, working great! :) now, what are the use cases for frequencies to fail, which I need to add in test cases (apart from arity check)? Since nothing extra as such is mentioned in official docs

vickychijwani commented 9 years ago

Just add any kind of test for correct arity but wrong type of argument. For example, (frequencies 1) should throw because 1 is not a collection. It ensures that we haven't forgotten to add a type assertion to the function.

koustuvsinha commented 9 years ago

Done. Now you can review and merge this PR! :)

vickychijwani commented 9 years ago

Awesome work, @koustuvsinha! Thanks for tackling a bunch of these!

koustuvsinha commented 9 years ago

@vickychijwani awesome work in adding Travis CI :+1: