Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggestion for fixing how to deal with symbols in merge of two specs #220

Merged
merged 2 commits into from
Apr 4, 2020

Conversation

wandersoncferreira
Copy link
Contributor

A proposed fix for issue #201 , this was the best attempt so far without using eval to accomplish the task.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 97.82% when pulling b321598 on wandersoncferreira:bugfix/merge-symbol into d043282 on metosin:master.

@wandersoncferreira
Copy link
Contributor Author

This is weird, I can't see what is wrong with the implementation for clojurescript. I rolledback every change and run the tests to see if something was broken before in my build and everything works fine, however if I add a simple (println "hi") statement inside this function, I already get a similar error:

;; ======================================================================
;; Testing with Phantom:


Testing spec-tools.core-test

ERROR: Exception outside tests:
ERROR: #object[TypeError TypeError: undefined is not an object (evaluating 'spec_tools.core_test.get_spec_test')]

ERROR: Stacktrace:
ERROR: file:///home/wand/spec-tools/target/out/spec_tools/doo_runner.js:376:1188
file:///home/wand/spec-tools/target/out/spec_tools/doo_runner.js:385:3
cljs$test$run_block@file:///home/wand/spec-tools/target/out/cljs/test.js:374:17
file:///home/wand/spec-tools/target/out/spec_tools/doo_runner.js:357:32
doo$runner$run_BANG_@file:///home/wand/spec-tools/target/out/doo/runner.js:60:50


global code
evaluateJavaScript@[native code]
evaluate@phantomjs://platform/webpage.js:390:39
phantomjs://code/phantom3043986299397057157.js:127:19
_onPageOpenFinished@phantomjs://platform/webpage.js:286:27
Subprocess failed

Which seems like I need to do something to test setup of phantomjs to rebuild or something. I'm not confortable with cljs yet so I would like some help to figure this out.

Thanks!

@ikitommi
Copy link
Member

I'll ask if someone understands what's going on here.

@wandersoncferreira
Copy link
Contributor Author

I completely misunderstood the problem here. The real problem is that var, and var-get does not work as expected in clojurescript.

I was already confused because I saw the reader macro #?(:clj .. I thought this function was only used in clojure and I didn't know why cljs was having problems with that. It seems like cljs also uses it, therefore I should verify how to do var, var-get in proper way in cljs.

@wandersoncferreira
Copy link
Contributor Author

wandersoncferreira commented Mar 28, 2020

I just changed var-get to use a deref when in cljs-env. It seems to work fine now.

Copy link
Member

@ikitommi ikitommi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test only runs with clojure, on clojurescript it still fails. Is there any way to get it working?

@wandersoncferreira
Copy link
Contributor Author

@ikitommi I can't reproduce any failure in the test suite now. I am running both ./scripts/test.sh clj|cljs without any error on this branch. I even merged master here to see if could have some conflict, but all works fine.

Can you send the error message?

@wandersoncferreira
Copy link
Contributor Author

wandersoncferreira commented Mar 29, 2020

Oh! I see at travis CI, can you fire a new build? That error was due to other merge fixed by #224 . It should be ok now.

@ikitommi
Copy link
Member

If you enable the new test for cljs too, you should see the error.

@wandersoncferreira
Copy link
Contributor Author

I didn't notice the reader macro there :x .. anyway, I got more information about the problem, in fact cljs is not very friendly and this error has many implications. I was talking to Mike Fikes about this issue and he pointed out to this SO about resolving symbols dynamically in cljs.

I can get from symbol qix to symbol spec-tools.core-test/qix and then I cannot transform it back to a var again, can't understand why I can't only call var on it. cljs.analyzer.api/resolver returns back another symbol instead of a var. The proposed solution by Fikes involves loading the ns-publics of the namespace where the symbol is defined and retrieving the var out of this map.

I don't like this solution that much because we cannot antecipate the size of the namespaces spec-tools users are going to load while using this.

@ikitommi
Copy link
Member

well, it didn't work on clj & cljs, now it works on clj. So I think it's better than before. Happy to merge if you think it's done.

@wandersoncferreira
Copy link
Contributor Author

I would merge and at least this PR has documenting some steps towards cljs as well if needed in future.

@ikitommi ikitommi merged commit b02175b into metosin:master Apr 4, 2020
@ikitommi
Copy link
Member

ikitommi commented Apr 4, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants