Clojurians Log v2

PR for range.

PR for partial.

Made a branch of yada with speculative tests. Found no errors. https://github.com/borkdude/yada/tree/speculative-tests

maybe we can do PRs to projects to make use of this project in their tests. 🙂

Seems like the partial PR has a conflict. On phone so can’t resolve.

did you already merge range? I’d like to make one tweak

maybe:

:args (s/or :arity-0 nil?
              :arity-1 (s/cat :end int?)
              :arity-2 (s/cat :start int? :end? int?)
              :arity-3 (s/cat :start int? :end int? :step int?))

Also, in the case of range, it would be an interesting exercise to see if we could name the args in the spec

Great minds think alike :)

doing it in one s/cat won’t work

it will work, but then e.g. the end argument will match the :start alternative

so I think this is the cleanest

I made the PRs independent of each other, so merge conflict would be expected, np

OK, if you could merge the new range spec @slipset, I’ll fix the other PR

@slipset both PRs are mergeable now

✅ and 🙏

🐼

For more detailed failure reports, use JUnit XML formatting. Failing command: clojure -A:test:cljstests Exit code: 1 Output: WARNING: Wrong number of args (4) passed to cljs.core/range at line 190 /home/circleci/repo/test/speculative/core_test.cljc Testing speculative.core-test ERROR in (range-test) (Error:NaN:NaN) expected: (range) actual: #error {:message "Call to #'cljs.core/range did not conform to spec:\nIn: [1] val: 1.7976931348623157e+308 fails at: [:args :arity-2 :end?] predicate: int?\nIn: [1] val: 1.7976931348623157e+308 fails at: [:args :arity-3 :end] predicate: int?\nval: (0 1.7976931348623157e+308 1) fails at: [:args :arity-0] predicate: nil?\nIn:

Seems like circle didn’t like all the range tests?

cljs fails, but plk succeeds

so I guess we need to assert number? instead of int?

PR should fix the tests

if you can wait, I’ll also write some tests with non-int numbers

done

thanks!

BTW, interesting that cljs fails and planck succeeds

I seem to remember there was a discussion in #cljs-dev (or a Jira ticket) with issues around the semantics of int? integer?

and their differences between clojure and clojurescript

For these kinds of things, if we end up supplying patches to ClojureScript, it has tests involving spec that run on both JVM and self-hosted ClojureScript that could be expanded in coverage.

wow:

(range 1.1 10.3 3.3)
;; => (1.1 4.4 7.7)

@slipset The int? / integer? stuff ended up being in here https://dev.clojure.org/jira/browse/CLJS-2921

what’s the tl;dr on int? integer? with regards to spec?

cljs accepts: (range \a \b \c) but that’s Hyrum I guess

chars are numbers 🙂

and in cljs chars don’t really make sense since javascript has no chars

apart from the implementation standpoint, clj doesn’t accept the input

I guess because on the JVM they’re boxed java.lang.Character instances

I think the TL;DR on int? / integer? for spec is that oftentimes you will want to use int? for argument specs. where the function isn't really an arbitrary-precision function, which integer? would imply. Also, int? was introduced around the time with spec with other useful functions for spec, like nat-int?, and friends. But, ClojureScript unfortunately will allow arbitrary precision goog integers to satisfy int?. You could just use int? presuming it will all be sorted in the end and that's probably good enough.

@slipset You said chars are numbers, but consider (number? \A) evaluates to false

(range \a \b \c) is an incorrect program, so, the frog could appear 🙂

This is what Stu meant when he said "Clojure is optimized for correct programs." To me, this means, that undefined behavior is allowed in the name of perf.

Very experimental branch with orchestra, just to see if I could get :ret checking to work: https://github.com/borkdude/speculative/tree/orchestra

probably too fragile, can’t get it working properly

@mfikes re chars/numbers I was wrong 🙂

In js chars are strings it seems:

one risk of writing :ret specs is that we don’t have a way to test them thoroughly atm

unless we use generative testing

so maybe we should try that instead 😉

one funny thing about spec: in Haskell you can say, the return value is a (possible infinite) list of ints. In spec you could assert it, but the checking never finishes

that’s why I chose seq? for :ret in range, and not something else

Places in ClojureScript that enforces the notion that characters are length-1 strings is in char and char?

@slipset just merged a trivial PR. Should I check if they have signed the CA before I merge, even with README improvements?

I suppose if you wrote a spec for char you would allow number? or char?

and if so, where do you check this?

You can check for the CA at https://www.clojure.org/community/contributors

But to be honest, I would love it if we took that list, put it in a file, and set up one of those GitHub CA-checking robots.

(This would serve as an excellent example of how this can be much much better than the current manual process used in Clojure/Script with JIRA.)

👍

If we did that, then we could announce that Speculative is ready for PRs.

Those robots look fairly easy to use. For example https://github.com/cla-assistant/cla-assistant / https://cla-assistant.io

could we also set the project up to test check PRs before they are merged?

Yes... I think React and Google projects end up looking like that. Let me find an example.

Oh, sorry I thought you were still asking about CAs.

There is configuration in the settings of a GitHub project where you can flip a switch and require CI to pass before a merge can be done.

(You protect the main branch and then add a rule that indicates "Require status checks to pass before merging")

that’s something for Erik to configure then

Is this CLA assistant something that we could use from a third party (the .io) domain, or do we have to setup something ourselves?

I don't know... there seem to be quite a different ones out there

we could write a simple script to begin with. just scrape the contributors page and check. with some webhook maybe?

or not even scrape, but download it statically into a list once every while

There is also the ability to simply set up a template that gets shown when a PR is submitted, which indicates you need to sign the Clojure CA if you haven't already.

That would be a good thing to start with. https://blog.github.com/2016-02-17-issue-and-pull-request-templates/

I think I might have set up protection of master now 🙂

Yeah, the template idea is a no-brainer. It essentially communicates what you want with respect to CA.

The robots would be slick, assuming they are easy and free to use.

Or, as you suggest Michiel, roll your own

@slipset I’ll test with my orchestra branch which fails. Don’t merge 😉

https://github.com/slipset/speculative/pull/40

🙂

It should be fairly easy to set this up actually.

Set up pr-building in circle,

Do a check before building that the gitub-user is listed on

https://www.clojure.org/community/contributors

fail if not.

this could be a simple planck script, since it has an http client right? 😛

And I believe we have plk available on our build-server 🙂

takes a while with that PR it seems

CIRCLE_PR_USERNAME

(->> (get "") :body (re-find #"slipset"))

(require '[planck.http :refer [get]])

Btw cjohansen has a CLA

(slurp "") also works

afk

yeah 🙂

It was a very pleasant side-effect when I implmented the http-stuff for planck-1.x back in the day.

@borkdude could you push another commit to the DONT’ MERGE thingy?

I pushed a commit to it (I think on your branch, and it failed the build.

https://github.com/slipset/clj-ca-checker

Very crude, but seems to work. Will add html parsing.

Seems to work.

$ clj -Sdeps '{:deps {clj-ca-checker {:git/url "" :sha "a0ea916eb606c048227f03f2c7d02ef851075f00"}}}' -m clj-ca-checker.clj-ca-checker "borkdude"
Borkdude@borkdude ~ $ echo $?
0
Borkdude@borkdude ~ $ clj -Sdeps '{:deps {clj-ca-checker {:git/url "" :sha "a0ea916eb606c048227f03f2c7d02ef851075f00"}}}' -m clj-ca-checker.clj-ca-checker "borkdude2"
Borkdude@borkdude ~ $ echo $?
1

@slipset Integrated it 🙂 https://github.com/slipset/speculative/commit/f1d23dd72aa49c0d836f56a81480403a0977d26e

isn’t it a bit weird that PRs can change the circle code though? they can still make the tests pass by tweaking it 😉

but we have to be careful in what we merge anyway, so that’s ok probably

cool, Alex is now chiming in: https://github.com/slipset/speculative/issues/42

Yeah, I guess you could tweak the circle-config, but then you’re obviously malicious.

That Alex is chiming in is super cool!

@alexmiller has joined the channel

I wonder if this inlining happens in user code as well: https://github.com/slipset/speculative/issues/44 Our tests do seem to work with / for example.

user=> (def non-zero? (complement zero?))
#'user/non-zero?
(s/fdef clojure.core//
          :args (s/or :single-arity (s/cat :non-zero-number
                                           (s/and number? non-zero?))
                      :multi-arity (s/cat :number
                                          number?
                                          :non-zero-numbers
                                          (s/+ (s/and number? non-zero?))))
          :ret number?)
clojure.core//
user=> (stest/instrument `/)
[clojure.core//]
user=> (/ 5 0)
Evaluation error at clojure.lang.Numbers.divide (Numbers.java:188). ArithmeticException Divide by zero

¯\(ツ)/¯

not sure if I’m crazy or you are

Prolly us :)

I’m going to suspect your test infrastructure until proven otherwise :)

hmm, who wrote that test 😉

I did

maybe some .cljc issue there. thanks for the heads up @alexmiller

it does seem like the test is getting run. If I comment it out, the test and assertion count goes down.

maybe we have to do something like this there: https://github.com/slipset/speculative/blob/master/test/speculative/test_utils.cljc#L24

oh, I can get the error to happen on (/ 0)

ah, inlining is set for arities >1

so only getting picked up for single arity

The reason I added the spec for / in the first place was basically as an answer to Tim Baldridge complaining about Clojures error messages on division by zero.

user=> (source /)
(defn /
  "If no denominators are supplied, returns 1/numerator,
  else returns numerator divided by all of the denominators."
  {:inline (nary-inline 'divide)
   :inline-arities >1?
   :added "1.0"}
  ([x] (/ 1 x))
  ([x y] (. clojure.lang.Numbers (divide x y)))
  ([x y & more]
   (reduce1 / (/ x y) more)))

And, although it’s to specific, since division accepts all numbers, it’s a cool example of stuff that’s (relatively) easy to do with spec, that would be fairly hard with a type system.

@slipset bleh

0 is a valid number :)

Yeah, but even just between Clojure and Clojurescript, division by zero seems to be somewhat undefined 😛

I even seem to remember a ticket on this in the Clojure Jira…

I don’t think it’s undefined in Clojure at all. I blame JavaScript for all problems. ;)

Oh, but I do. All the time. Unfortunately my cow-workers are getting tired of that 😕

Lucky thing I only checked for integers:

https://dev.clojure.org/jira/browse/CLJ-1142

Michiel hot tip: If you prefix your commit msgs with [Fix #41], you automatically close issue 41 and you make me, who’s slightly OCD wrt commit mgs, very happy :)