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

Translated loom.graph to CLJS #76

Closed
wants to merge 9 commits into from
Closed

Conversation

Hendekagon
Copy link

hi, bit of a messy pull request this one (file mode changes, don't know how that happened) but please review loom.graph -- I translated the code to CLJS by changing (defrecord.. & (extend... to just (defrecord using a horrid macro. I don't expect you to accept this PR without some discussion as the code is messy, but it appears to work. I also added various CLJS implementations of expressions where needed in other namespaces

@Hendekagon
Copy link
Author

changed to #queue [], pprinted new function implementations in loom.graph, better formatting

@brianfay
Copy link

Any status on this PR? I'm starting a cljs app where I would love to use loom. My use case is hopefully simple enough to use stuartsierra/dependency, but it would be very cool to have loom as an option.

@aysylu
Copy link
Owner

aysylu commented Feb 1, 2016

Hi @Hendekagon, would you mind cleaning up this PR and reverting any changes that are not intended to be part of this? Also, if you could please merge the conflicts, that'd be great. Thanks!

@Hendekagon
Copy link
Author

hi, hopefully I caught all the conflicts & this passes all the tests now

@pataprogramming
Copy link
Collaborator

Thanks very much, @Hendekagon. I'm reviewing all pending pull requests and issues, and this one is at the top of the list.

@pataprogramming pataprogramming self-assigned this Mar 20, 2016
@pataprogramming
Copy link
Collaborator

Hi, @Hendekagon, and thanks again for putting the work into adding this support. My apologies that it's taken me so long to sit down and give this PR a thorough going-over. I have a couple of requests/questions:

  1. This PR has a large number of mode changes to files. Can you please revert them to 644?
  2. You've made changes to the core protocols, moving the single-arity versions of successors, predecessors, etc., from the wrappers section into the protocols, and changing their names from the starred versions. Is there a technical CLJC/CLJS requirement for this? I'm reluctant to make breaking changes to the core API, and this would affect anyone who has implented their own versions of the protocols.

@Hendekagon
Copy link
Author

hi, this PR is from a fork of an earlier version of loom before the protocols were changed to include the starred fns, so it's now out-of-date

@ghost
Copy link

ghost commented Mar 25, 2016

Any hope of this being resolved?
I'm currently implementing a force based graph layout lib for of cljs and clj.
And it would be really nice to base it on loom.

@Hendekagon
Copy link
Author

problem is, that the implementations of starred functions like successors* include single-arity definitions that aren't in the protocols

@pataprogramming
Copy link
Collaborator

I'd love to get this merged, if someone wants to tackle updating the PR to match the current protocols. I'll get to it at some point, but if someone has free cycles now, it would be a great contribution.

@Hendekagon
Copy link
Author

I'm happy to fix it up, but the way it's written atm won't work with (defrecord) unless we add the other arity fn definitions of starred fns to the protocols, afaiu ?

@turbopape
Copy link

Hey guys.
I myself used macros etc to try to translate extend to extend-type calls.
too messy, and even if I got it to compile and load, still get artity mismatches in the protocols which prevented me to continue work on it...
Besides, futures etc prevent a pure transparent rewrite, I used a third-part lib wrapping js promises, but it isn't clean.
Can we rewrite the lib to use just common ground between clj and cljs, so I can add cljs part?

Thanks.

Rafik

@turbopape turbopape mentioned this pull request Oct 20, 2016
10 tasks
@pataprogramming
Copy link
Collaborator

Superceded by #91.

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.

6 participants