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

Eliminate compiler warnings in ClojureScript #19

Merged
merged 3 commits into from
May 29, 2018
Merged

Eliminate compiler warnings in ClojureScript #19

merged 3 commits into from
May 29, 2018

Conversation

chpill
Copy link
Contributor

@chpill chpill commented May 15, 2018

Following up on #18

The warning about Throwable is very easy to get rid of (see diff).

The warning about the *file* dynamic variable less so... I don't know much about it except its doc. All I've learned for now is that earmuffs are really inconvenient when you look for them in search engines 😅 . I will dig deeper in the following days.

By the way, I use the following command to test the self-hosted clojurescript case (very primitive but convenient):

# at the root of the project
lumo --classpath src/ \
     --eval "(require '[sc.api :as sc]) (let [a 42] (sc/spy)) (sc/letsc [1 -1] (assert (= a 42)))"

@vvvvalvalval
Copy link
Owner

vvvvalvalval commented May 17, 2018

Re: *file*, I guess we could just use reader conditionals as well (AFAICT file is only defined for Clojure JVM).

@chpill
Copy link
Contributor Author

chpill commented May 18, 2018

As expected, this one was a bit more tricky, but it seems we now capture the file name correctly in all cases.

For self-hosted cljs, I tested with: lumo --classpath src/ --init lab/sc/lab/cljs/example.cljs (i have not tried with planck yet).

For the latest jvm cljs, I tested by adding the following deps.edn at the root of the project:

{:paths ["src"]
 :deps {org.clojure/clojurescript {:mvn/version "1.10.238"}}}

and running the following: clj --main cljs.main lab/sc/lab/cljs/example.cljs

We should probably write a proper cljs test file and run it in different environments, but that's a story for another PR :)

@vvvvalvalval
Copy link
Owner

Good, thanks!

I added an issue for CLJS testing: #22

@vvvvalvalval
Copy link
Owner

@chpill Could you add an entry to the CHANGELOG? After that, ready to merge.

@chpill
Copy link
Contributor Author

chpill commented May 23, 2018

@vvvvalvalval 👌

@vvvvalvalval vvvvalvalval merged commit 69fc965 into vvvvalvalval:master May 29, 2018
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.

2 participants