PersistentList support

[muhuk]

Re #142, sequence monad for PersistentList's.

• cats.builtin-spec/sequence-applicative-identity fails. I can't understand why, the implementation seems to be correct. I printed even out (eq app (m/fapply (m/pure ctx identity) app)), it's true. I would appreciate if you can take a look. FWIW it fails both during clj & cljs runs.
• Didn't rigorously benchmark, but quick tests with (time) shows using loops is about 7 times faster than forcing LazySeq's. My understanding is LazySeq in, LazySeq out and PersistentList in, PersistentList out. So I force everything.

Please take a look and let me know if there are any changes needed.

[muhuk]

The failing test with the failing node, simplified:

(defn f [self av]
  (loop [[f & fs :as fcoll] self
             [v & vs :as vcoll] av
             ys '()]
        (if (or (empty? fcoll)
                (empty? vcoll))
          (reverse ys)
          (recur fs
                 vs
                 (cons (f v) ys)))))


(def input '({} ["" -1.5 -Infinity "�\f�"] {(-2 -1) (), {} {#uuid "ebf99b14-3384-4036-b388-130e8e1f917e" 3}} {{{a_P :S4w, 0 :+:3:_X:yl*f} [], (:L_.!6?2E.?/N?_hB) (WIo+.hDH9.M!Q/hO)} []}))

(def ss (repeat (count input) identity))

(= input (f ss input))

[muhuk]

I've found the problem. Working on a fix now.

[muhuk]

This one looks OK.

Please take a look. Because of doing everything eagerly there is a bit of complexity involved (as opposed to complexity inside clojure.core). I moved it behind the macros for now. If you think it's better, I can unpack them again.

Haven't done more benchmarks, but I'd be happy to do them if you have doubts if the performance gained is worth the complexity.

[ghost]

Please take a look. Because of doing everything eagerly there is a bit of complexity involved (as opposed to complexity inside clojure.core). I moved it behind the macros for now. If you think it's better, I can unpack them again.

I think that for this simple cases there's no need to introduce more macros, I'm fine with a bit more code in the implementation. We try to introduce macros only for new syntactic abstractions like mlet or alet.

Haven't done more benchmarks, but I'd be happy to do them if you have doubts if the performance gained is worth the complexity.

Add them if you want to and have time but I'm fine with merging this contribution without them.

[muhuk]

I think that for this simple cases there's no need to introduce more macros, I'm fine with a bit more code in the implementation. We try to introduce macros only for new syntactic abstractions like mlet or alet.

Will do. I wasn't happy with the CL-like naming anyway.

Add them if you want to and have time but I'm fine with merging this contribution without them.

I was thinking of tackling the undecipherible context not found exception next. Basically asserting this is Contextual before calling -get-context or something like that. I didn't know about cats/dev/benchmarks.clj. So I am thinking about a benchmark between lazy sequences & IPersistentLists. This is to be fair; evaluating a IPersistentList lazily and then casting it back into a IPersistentList is itself redundant work. (I'll create a ticket first for the -get-context thing.)

[yurrriq]

It seems to me this should be a (private) function, not a macro, and live in builtin.cljc.

[yurrriq]

Other than my inline nitpicks, this looks great. Thanks, @muhuk.

[yurrriq]

I might change this to:

Like (do all (map f v)) but eager.

[muhuk]

OMG @yurrriq you have hurt my fragile ego. I give up. I give up Clojure. I give up open source. I won't even use a computer ever again.

jk.

Thanks for the detailed review. I see no nitpicks. What I don't respond to below, I'll fix in the next PR.

It seems to me this should be a (private) function, not a macro, and live in builtin.cljc.

Maybe I'm missing something, but why not something like this instead?

No, you are not missing anything. I am not happy with the macros either. Following @dialelo's advice I was going to unpack those macros directly into the code. I would rather avoid function calling here. For readability reasons rather than performance. Let me give it a try that way and we can discuss.

[yurrriq]

Haha you really got me. I was getting nervous up until the jk...

Avoiding extra function calls for legibility sounds good to me, though it might be nice to pull out redundant code into functions. Your call. My general threshold is three repetitions before I reach for further abstraction, but maybe that doesn't apply here.

[yurrriq]

+1e6 for cleaning up the context error messages. That issue seems to get raised fairly often by users.

[muhuk]

JS tests are failing in two JDKs. They run fine on my machine though.

Clojure 1.7.0
OpenJDK 64-Bit Server VM 1.7.0_91-b02

sadface

[yurrriq]

Odd. This is a bit out of my wheelhouse, but I've copied (what seems to me to be) the pertinent output here for discussion.

$ ./.travis/test-cljs.sh
...
Applying optimizations :advanced to 84 sources
Killed
module.js:340
    throw err;
          ^
Error: Cannot find module '/home/travis/build/funcool/cats/out/tests.js'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:929:3
...

[muhuk]

I tried, unsuccessfully, upgrading cljs.

If fails during compilation (no timing output). So it wouldn't surprise me if we find no tests.js file when we look at the build dir.

I am out of ideas.

[ghost]

Minor nitpick: we should indent this properly.

[muhuk]

Done.

[ghost]

Strange, I haven't been able to reproduce the failure and I don't see anything that could've caused ClojureScript compilation to fail. @niwinz can you take a look?

[muhuk]

Should I give up on this? Like @dialelo, I can't reproduce the failure that happens in CI.

[niwinz]

Do not worry, this happens because of high memory usage of advanced compilations. We should just disable advanced compilations in tests and the problem will disappear.

[muhuk]

Thanks @niwinz . :simple is no good, but :none seems to do the trick.

[yurrriq]

Nice!

[muhuk]

Can somebody merge this?

Or tell me what issues it still has, if any?

[ghost]

Thanks for your time @muhuk!

[muhuk]

Thanks @dialelo.

So, just to be sure; if I want to make other contributions, I should ping you guys with a call to action at every step? Or is there a process with less friction, that I'm failing to see?

[yurrriq]

I hope you want to make other contributions!

It seems to me this is most likely a case of other responsibilities eating up time. Feel free to chime in @dialelo and @niwinz, but I'd say just make a PR that accomplishes a goal the way you want to and then ping someone if we're too slow to review. I'm not sure about the other guys, but sometimes my notifications get buried or I get pulled away by other work.

Thanks again for your time and effort, @muhuk, and I look forward to seeing what you do next. 😄

[ghost]

As @yurrriq points out is a matter of not having time, and sometimes sadly notifications get buried in a pile of other stuff to do.

If you want to make other contributions please do so, no need for a call to action in every step. Make the changes that you want in the library and ping us when you think it's time to review and merge.

Sorry for the ping-pong interaction in your last PR and thanks for making me see that is a lot of friction that may discourage further contributions. Don't hesitate to propose improvements to our workflow, there's a lot we could do to encourage more contributions.