Fork me on GitHub
#cljs-dev
<
2018-01-03
>
mfikes12:01:22

I suppose ^ would be a way to solve https://dev.clojure.org/jira/browse/CLJS-2455 I wonder if there is an alternative, though, by having cljs.core/Eduction satisfy some appropriate protocol. :thinking_face:

rauh12:01:10

I think nth should check for ISeqable instead of ISeq, no?

mfikes12:01:43

linear-traversal-nth does call seq, so that argument makes sense to me.

mfikes13:01:27

Even in the cond branch for (seq coll), where it calls next, that will ultimately call rest, which calls seq.

mfikes13:01:12

^ this is evidently part of an hack session of sorts leading to support for self hosted ClojureScript. Wow. https://github.com/wbrown/clojurescript/pull/4

mfikes13:01:35

Or am I reading GitHub backwards?

dnolen13:01:48

@mfikes I think that’s an older effort?

mfikes13:01:42

Yeah, just trying to dig up some rationale for ISeq vs. ISeqable. Perhaps it was part of a mad hack session, which makes it harder to dig into. But I'm speculating that I might be wrong.

dnolen13:01:57

well the former is the actual interface, and the other is coercion

mfikes13:01:38

I see the above link in GitHub, which may simply be because I cloned something, and it is actually referring to a merge in the other direction (from ClojureScript master to some experimental branch).

dnolen13:01:33

I think the idea was that nth shouldn’t coerce it’s argument

mfikes13:01:30

OK, the argument would be: nth only works on types that directly satisfy some needed interface. (That's an appealing argument IMHO.)

dnolen13:01:58

yeah looking at the Clojure behavior this seems to match nth there

dnolen13:01:06

the argument must already be Sequential

mfikes13:01:42

I'm trying to see how Clojure's deftype Eduction ends up being Sequential. Maybe it is the Util.ret1(coll, coll = null) call? Hrm.

mfikes13:01:01

Oh, you are saying Clojure doesn't coerce either. Got it. 🙂 Then the question becomes, how does nth work on Eduction in Clojure?

dnolen13:01:37

Eduction has a Sequential marker

mfikes13:01:56

Ahh. Damn it. At the bottom! Hah.

dnolen13:01:12

looking at the Clojure code a bit more closely that’s the only time we should coerce via seq, if we have Sequential

dnolen13:01:26

seq works on Eduction because Eduction is Iterable

mfikes13:01:38

Clojure's logic seems to be, if translated to ClojureScript: If a type satisfies ISequential, then nth is supported on that type. But try to coerce via seq, and if that coercion fails, then fail with "Index out of bounds".

rauh14:01:29

To be fair, Clojure's nth implemenation is 10+ years old. Before Seqable was a thing. Nowadays, clojure's nth could probably just use canSeq

mfikes14:01:19

As a test, I’m going to run a change from (implements? ISeq coll) to (or (implements? ISeq coll) (implements? ISequential coll)) through Coal Mine to see if if it finds anything of interest. This change is sufficient to make both (nth (eduction [:x]) 0) and (nth (eduction [:x]) 1 in ClojureScript behave like Clojure.

mfikes14:01:09

As expected, it would be hard for any code in Coal Mine to provoke a failure with this change. I’ll capture the logic / rational, and attach a candidate patch to https://dev.clojure.org/jira/browse/CLJS-2455

mfikes14:01:51

Thanks @dnolen, @rauh. TBH, apart from the fact that it interrupts, working through an issue here can be effective compared to conversations in JIRA. 🙂

mfikes15:01:00

With the new map-entry? predicate on master, (map-entry? (first {:a 1})) differs from Clojure (it’s being honest, at least), but I’m curious if there is a ticket to address the underlying issue

mfikes15:01:34

(I’m not seeing one.)

dnolen15:01:22

I think we made the most conservative set of changes first, we need to change the map types to return MapEntry instead of vectors now

mfikes15:01:02

I’ll log an enhancement ticket (I’m presuming one doesn’t exist)

dnolen15:01:24

I don’t think one exists? thanks

dnolen15:01:37

(maybe check first though)

mfikes15:01:09

Yeah. That’s what I’m doing… don’t want to add a dup. I’ll check and if I can’t find one, I’ll log a presumably fresh one.

mfikes17:01:08

Ugh. If anyone has ideas on https://dev.clojure.org/jira/browse/CLJS-2457, please share.

mfikes17:01:39

I’m wondering if something is botching the test framework… doing more work to try to isolate where this bizarre behavior is coming from.

mfikes18:01:19

I seem to vaguely recall this coming up before. In out unit tests, (prn (seq #js {:a 1 :b 2})) will print (["a" 1] ["b" 2]). WTF.

dnolen20:01:30

@mfikes I think this is because we do this as one of the tests? Extend object to seqable?

dnolen20:01:38

probably a protocol test

mfikes20:01:02

Yeah… that would make sense. I’ll see if I can find that culprit and work around it.

dnolen20:01:32

it’s also probably a bad test 🙂

mfikes20:01:45

Yeah… perhaps I can revise it to accomplish the same goal without “polluting”

dnolen20:01:49

@mfikes we could also force this test to come last?

mfikes20:01:05

Yeah… I was thinking the same. It seems like an important test.