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

Make loom Clojure[Script] portable #91

Merged
merged 5 commits into from
Jan 11, 2017
Merged

Conversation

cemerick
Copy link
Contributor

(This PR supersedes PR #76, and when merged should close #45.)

While I'm using a small subset of loom's functionality, I do have the results of this work running in the browser in a nontrivial application -- so I think it's in decent shape, beyond the project's own tests.

If you'd like to try it out in a ClojureScript app/environment, just add [org.clojars.cemerick/loom "0.6.1-SNAPSHOT"] to your dependencies.

Bits that remain Clojure-only:

  • loom.gen (migrating to use test.check's generation facilities would be easier than trying to make what's there now useful in ClojureScript)
  • loom.alg-generic/bf-path-bi (no futures in JS)
  • loom.io (fundamentally unportable utilities)

Aside from the above caveats re: portability, and the extended documentation of the extend hack/solution in loom.cljs, I'd wanted to note that I ended up needing to tweak a number of tests to account for different ordering implementation details in ClojureScript (at the moment!) vs. Clojure. See loom.test.alg/bipartite-test for an example; there are many possible solutions for many of the algorithms in loom, so simple equality assertions aren't sufficient to really establish correctness. Representing expected values as sets (and transforming results similarly) can help, but this will be a persistent problem given the existing test approach.

Finally, please note the new :aliases in project.clj. A quick skimming of doo's readme would be good if anyone plans on running the test suite in ClojureScript in an interactive setting. (tl;dr: lein do clean, doo node)

I hope this meets expectations. Let me know what your questions are.

…an CLJS compile while Clojure tests stay green.

Bits that remain Clojure-only:

* loom.gen (migrating to use test.check's generation facilities would be easier
  than trying to make what's there now useful in ClojureScript)
* loom.alg-generic/bf-path-bi (no futures in JS)
* loom.io (fundamentally unportable utilities)
@cemerick
Copy link
Contributor Author

Just now noticed that the CI failed, turns out ClojureScript doesn't like OpenJDK 6 (7+ is required). PR now includes an updated travis config, swapping the openjdk6 target for JDK8.

@pataprogramming
Copy link
Collaborator

Thanks very much for this excellent pull request! I'll begin reviewing this so we can get it merged and a new release cut

@viebel
Copy link

viebel commented Oct 25, 2016

@pataprogramming @cemerick it works like a charm inside KLIPSE. Take a look at this interactive gist

@viebel
Copy link

viebel commented Oct 25, 2016

Once this is merged, you can make the documentation interactive using codox klipse theme.

@cemerick cemerick mentioned this pull request Oct 25, 2016
10 tasks
@dustingetz
Copy link

Thanks @cemerick for this work. Fork is working great for me.

@viebel
Copy link

viebel commented Dec 5, 2016

@cemerick
Copy link
Contributor Author

👋 Just saying hi, thought I'd check in and see if there's any further help needed here. I'm happy enough using my own build as necessary, but if there's tweaking to be done here, I'd love to take care of it while the work is still somewhat front-of-mind.

@aysylu
Copy link
Owner

aysylu commented Dec 17, 2016

Hey @cemerick, sorry about the delay and thanks for the great PR! Either @pataprogramming or I will respond to this PR this weekend.

@cemerick
Copy link
Contributor Author

Please, no apologies. I know the OSS drill, definitely don't want to change any priorities, etc. Enjoy this weekend long before you look at this PR. ❤️

@pataprogramming
Copy link
Collaborator

I've finally been able to sit down and spend the detailed time with this pull request that it deserves.

First: Abject apologies to @cemerick and the community of Loom users that it's taken this long!

Second: Thank you, @cemerick, for a brilliantly clean and well-written pull request. My own use of ClojureScript has been relatively limited, and I've learned quite a number of useful things on how to structure projects while walking through your changes.

Third: I'm accepting this pull request as submitted. If any Loom users have additional feedback on the design or run into problems, please submit them as issues. If there's no significant pushback, we'll cut a new release soon.

@pataprogramming pataprogramming merged commit daf5ec2 into aysylu:master Jan 11, 2017
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.

clojurescript support
5 participants