Fork me on GitHub
#cljs-dev
<
2019-01-07
>
borkdude13:01:35

why doesn’t MapEntry implement ISeq directly?

bronsa13:01:10

because they are vectors

bronsa13:01:19

and vectors are not seqs

borkdude13:01:19

in ClojureScript I mean

bronsa13:01:41

it should be the same

bronsa13:01:46

by vectors I mean IPV not PV

borkdude13:01:40

what does the implementation matter as long as you implement the protocol faithfully?

bronsa13:01:53

there's a bit of a diamond problem in certain paths if you implement both ISeq and IPersistentVector/Indexed in clojure

bronsa13:01:11

don't know if the same problem exists in cljs too but I wouldn't be surprised if it did

borkdude13:01:37

k, just wondering about it 🙂

borkdude13:01:18

now that you’re awake, do you think it would be likely to receive an argument to apply that does not implement INext?

bronsa13:01:19

but usually the pattern is that you make things seqable and then the seq object is just a seq and nothing else

bronsa13:01:11

I'm not sure re: INext, I don't really remember how apply works in cljs

tmulvaney15:01:21

@borkdude I think I asked dnolen about ISeq and INext awhile back

tmulvaney15:01:53

Because in CLJ ISeq has first/next/more as methods, where as ISeq in CLJS has first/rest as methods.

tmulvaney15:01:17

I can't remember why it was different but I think the upshot was: in CLJS if implementing ISeq one should also implement INext to handle the next method. I may be totally misrembering though...

borkdude15:01:20

@tmulvaney I’m asking specifically for this patch: https://dev.clojure.org/jira/browse/CLJS-3023 So far I haven’t encountered any problems by rewriting (next ...) to (-next ...) in apply-to-simple. It also passes the Canary tests. I think you could artifically create an ISeq instance that does not implement INext, but I wonder if this will ever happen in practice.

tmulvaney15:01:59

Yeah i think all the core datastructures have INext where it's needed. It would be external data types from libraries etc which may not implement INext.

dnolen15:01:47

@borkdude I would say you cannot skip next

dnolen15:01:06

next handles the case where you don't have INext

dnolen15:01:25

if I recall, ClojureScript didn't ship with INext protocol

dnolen15:01:52

and it was added separately

borkdude15:01:22

yes, that’s the question I was wondering about. Alternatively I wrote a version of next that skips the nil? call, since that has already been established when calling next in apply-to-simple.

borkdude15:01:54

it fixes the issue, but not sure it’s worth it. also, the performance gain is really nothing. maybe I should just close the ticket.

borkdude15:01:19

I assume you meant: libraries that targeted older versions of CLJS and have their own data structures that implement ISeq but not INext could potentially give problems in combination with apply

dnolen15:01:31

it doesn't matter if it's old or not

dnolen15:01:40

you don't need to implement INext due to next

borkdude16:01:04

yeah, I meant, when this protocol didn’t exist, users didn’t even have the choice, so it’s likely those exist out there

borkdude16:01:25

this could be used in apply-to-simple instead of next:

(defn- ^seq next*
    (if (implements? INext coll)
      (-next ^not-native coll)
      (seq (rest coll))))
it skips the nil? check and also fixes the issue.

dnolen16:01:18

I'm ok with that

dnolen16:01:09

@borkdude one other thing

dnolen16:01:29

I think your change to the keyword clojure.test.check will cause unnecessary headaches in the next release

dnolen16:01:40

I think we should probably support both the old keyword and the new one

borkdude16:01:51

makes sense

borkdude16:01:38

so accept both as input, but if people use the the new one, the check results should also use the new one, else the old one?

dnolen16:01:26

I mean if the keyword is internal just convert to the new way

dnolen16:01:43

but allow the old key as config

borkdude16:01:41

so the output must always be in the same format? this will still be breaking

borkdude16:01:31

if people do something like: (every? #(:pass? (get % :clojure.test.check/ret)) stc-result)

borkdude16:01:11

I’ll see if I can maintain full compatibility

dnolen16:01:44

that would be best

borkdude16:01:37

updated CLJS-3023 as discussed

mfikes17:01:46

I haven't yet looked at the stackoverflow in https://dev.clojure.org/jira/browse/CLJS-3030 but I've unassigned it from me (anyone should feel free to take a whack at that one)

borkdude18:01:10

sorry, the patch contains old and new code… will update

borkdude18:01:34

fixed, sorry for the hiccup

dnolen21:01:04

@borkdude looks like 3023 needs a rebase?

borkdude21:01:51

yeah, I’ll do it now

borkdude21:01:06

@mfikes maybe the text about the keyword change for passing clojure.test.check options should mentioned that the old keyword is still supported as backward compatibility

borkdude21:01:15

or not, people won’t notice that it has changed if they don’t read the release notes 🙂

mfikes21:01:30

Yeah, I’ll update. We could almost remove the note, but I’ll try revising the language to see how that works.

borkdude21:01:23

@mfikes there is one interesting update though. You now have to to require clojure.test.check and clojure.test.properties yourself, because this is not done by clojure.spec.test.alpha anymore (it’s made lazy)

borkdude21:01:45

but if you don’t, you’ll get an error message about this

mfikes21:01:55

Ok—I’ll add some copy surrounding that

mfikes21:01:44

Or feel free to make a PR to the PR if I don’t get to it right away

borkdude22:01:22

$ clj -m cljs.main -re node
cljs.user=> (require '[clojure.spec.test.alpha :as stest])
cljs.user=> (require '[clojure.spec.alpha :as s])
cljs.user=> (s/fdef clojure.core/inc :args (s/cat :n number?))
cljs.core/inc
cljs.user=> (stest/instrument)
[cljs.core/inc]
cljs.user=> (stest/check `inc)
Execution error (Error) at (<cljs repl>:1).
Require clojure.test.check and clojure.test.check.properties before calling check.

borkdude22:01:09

Feel free to reword anything I wrote as English is not my first language.