Add lumo.repl/get-arglists #88

arichiardi · Feb 26, 2017

Hi Antonio, I added this utility function because inf-clojure needs it.

Oddily, the tests are not working on my machine, but the same in the yarn dev repl works:

cljs.user=> (require '[cljs.test :as test :refer-macros [is]])
nil
cljs.user=> (is (= (lumo/get-arglists "cljs.core/map") '([f] [f coll] [f c1 c2] [f c1 c2 c3] [f c1 c2 c3 & colls])))
true

I could not figure out why, but lumo.repl/resolve-var calls to cljs.analyzer both fail with #object[Error Error: No protocol method IDeref.-deref defined for type null: ]. Again, this happenes only in boot test, maybe it is running with another version of ClojureScript.

In any case, I'll be waiting for your comments 😉

anmonteiro · Feb 26, 2017

That doesn't make sense to me, but I just pushed an update to CLJS. can you rebase on current master to see if that fixes it?

arichiardi · Feb 27, 2017

A test still does not pass:

Testing lumo.repl-tests

FAIL in (test-get-arglists) (at /home/arichiardi/.boot/cache/tmp/home/arichiardi/git/lumo/ie3/-dde9ud/cljs_test/generated_test_suite.out/cljs/test.cljs:469:9)
expected: (= (lumo/get-arglists "when") (quote ([test & body])))
  actual: (not (= nil ([test & body])))

Probably resolve-var cannot resolve it, I am checking.

arichiardi · Feb 27, 2017

src/cljs/snapshot/lumo/repl.cljs

+    (if-not (:macro var)
+      (:arglists var)
+      ;; filtering &env &form for macros
+      (map #(filterv (complement '#{&env &form}) %)


I had to do this because apparently in the real repl that symbols have &env and `&form as arglists, while in the tests they don't. I know that I fixed a symptom and I haven't investigated the cause but hopefully we'll figure it out at some point.

For reference on this, I opened CLJS-1958, but it is probably better to have it here, not upstream I guess (see comment).

After convo on Slack, this solution will be implemented instead of this ugly hack.

arichiardi · Feb 27, 2017

test/lumo/repl_tests.cljs

+  (is (= (lumo/get-arglists "cljs.core/map") '([f] [f coll] [f c1 c2] [f c1 c2 c3] [f c1 c2 c3 & colls])))
+  (is (= (lumo/get-arglists "map") '([f] [f coll] [f c1 c2] [f c1 c2 c3] [f c1 c2 c3 & colls])))
+  (is (= (lumo/get-arglists "clojure.core/when") '([test & body])))
+  ;; (is (= (lumo/get-arglists "when") '([test & body]))) ;; cannot test it calling lumo.repl/init works in tests


Just so that I don't forget, the problem here was that we don't have this key for when in the compiler env unless we are able to load the compiler cache (call to lumo.repl/init). We cannot load the compiler cache because lumo.repl/init triggers a $$LUMO_GLOBALS.load is not a function.

arichiardi · Mar 3, 2017

I fixed this with the approach in doc* but again without analysis cache the tests are not passing. It is a pity but unless I hook up the .load function there is nothing I can do I think.

anmonteiro · Mar 3, 2017

@arichiardi I don't see those changes. Did you forget to push?

arichiardi · Mar 3, 2017

No but soon I will (push not forget 😀)

anmonteiro · Mar 3, 2017

So one thing you can do is just call cljs.js/load-core-analysis-cache! with a dump of the when macro for testing purposes only.

arichiardi · Mar 3, 2017

Probably for this unit test yes, I was wondering if more broadly it is worth just setting the load from the hardcoded path to the cache...Generated in a fixture and dumped in a temp folder...Probably a way bigger task but worth doing at some point.

anmonteiro · Mar 3, 2017

Sure but we don't wanna do that for now. Seems like a too big cost for very little benefit

arichiardi · Mar 3, 2017

Agree

arichiardi · Mar 8, 2017

Finally ready for review! I ended up implementing the full analysis cache load because it was relatively straight forward. I am looking forward to your comments 👍

anmonteiro · Mar 8, 2017

.gitignore

+lumo-cache/
+third_party/


this is not needed anymore, better to keep the diff clean and not touch this file

anmonteiro · Mar 8, 2017

test/lumo/test_util.cljs

+    (catch :default e nil)))
+
+(def cljs-core-cache-path "generated_test_suite.out/cljs/core.cljs.cache.json")
+(def cljs-core-macros-cache-path "generated_test_suite.out/cljs/core$macros.cljc.cache.json")


not a big deal, but this is relying on implementation details of boot-cljs-test which is subject to change. Can we add an explicit ids option to the test task in build.boot? Something like :ids #{"test_out"}

yes we can, but the path will always be hardcoded anyways right?

yes but at least we have control over its name

sure will do

This patch adds a :once fixture which loads the analysis cache for `cljs.core` so that it will be easier to test features that need macro ast data to be loaded.

arichiardi · Mar 8, 2017

test/lumo/test_util.cljs

+    (catch :default e nil)))
+
+;; For the cache source folder, the test id needs to become:
+;;   lumo_test/test_suite => test_suite.out/


put a comment in there

arichiardi force-pushed the get-arglists branch from 67dca47 to 1e37412 Compare February 27, 2017 00:37

arichiardi force-pushed the get-arglists branch 2 times, most recently from d337330 to ce71a64 Compare February 27, 2017 02:06

arichiardi commented Feb 27, 2017

View reviewed changes

arichiardi mentioned this pull request Mar 6, 2017

Support REPL flavors and Lumo clojure-emacs/inf-clojure#44

Merged

6 tasks

arichiardi force-pushed the get-arglists branch from ce71a64 to b198a55 Compare March 8, 2017 00:35

Add lumo.repl/get-arglists

cd8552f

arichiardi force-pushed the get-arglists branch 2 times, most recently from a4c89c3 to 86da71e Compare March 8, 2017 00:44

anmonteiro suggested changes Mar 8, 2017

View reviewed changes

arichiardi added 2 commits March 7, 2017 17:25

Load analysis cache as fixture in tests

ae9bd43

This patch adds a :once fixture which loads the analysis cache for `cljs.core` so that it will be easier to test features that need macro ast data to be loaded.

Add tests for lumo.repl/get-arglists

8331c32

arichiardi force-pushed the get-arglists branch from 86da71e to 8331c32 Compare March 8, 2017 01:28

arichiardi commented Mar 8, 2017

View reviewed changes

anmonteiro approved these changes Mar 8, 2017

View reviewed changes

anmonteiro merged commit ad0a187 into anmonteiro:master Mar 8, 2017

arichiardi deleted the get-arglists branch March 8, 2017 05:58

		lumo-cache/
		third_party/

Search code, repositories, users, issues, pull requests...

Add lumo.repl/get-arglists #88

Add lumo.repl/get-arglists #88

Uh oh!

Conversation

arichiardi commented Feb 26, 2017

Uh oh!

anmonteiro commented Feb 26, 2017

Uh oh!

arichiardi commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichiardi commented Mar 3, 2017

Uh oh!

anmonteiro commented Mar 3, 2017

Uh oh!

arichiardi commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmonteiro commented Mar 3, 2017

Uh oh!

arichiardi commented Mar 3, 2017

Uh oh!

anmonteiro commented Mar 3, 2017

Uh oh!

arichiardi commented Mar 3, 2017

Uh oh!

arichiardi commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arichiardi commented Mar 3, 2017 •

edited

Loading