Skip to content

Rum#7

Open
bamarco wants to merge 13 commits intodenistakeda:masterfrom
bamarco:rum
Open

Rum#7
bamarco wants to merge 13 commits intodenistakeda:masterfrom
bamarco:rum

Conversation

@bamarco
Copy link
Copy Markdown

@bamarco bamarco commented Feb 11, 2020

rum plugin

@bamarco
Copy link
Copy Markdown
Author

bamarco commented Feb 11, 2020

@denistakeda Maybe now it will work?

@denistakeda
Copy link
Copy Markdown
Owner

@bamarco . Unfortunately, I still can not merge it :-( A bit later I'll move the differences manually to another PR and provide it to you for review.

Comment thread src/posh/reagent.cljc
[reagent.ratom :as ra]))

(defn derive-reaction [reactions key f & local-mixin]
(apply ra/make-reaction
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hello @bamarco . I tried to manually recreate your PR, but run into the issue. The thing is, it looks like there is no such thing as reagent.ratom/make-reaction in the clojure version of namespace https://github.com/reagent-project/reagent/blob/master/src/reagent/ratom.clj
So when I tried to run tests with this code they failed to compile with a message:

Syntax error compiling at (posh/reagent.cljc:9:3).
No such var: ra/make-reaction

Could you please have a look at this?

@fpischedda
Copy link
Copy Markdown

@denistakeda

Hi! I would like to help integrate Posh with Rum, is this PR still relevant?

If I am not mistaken you started to cleanup this PR but then got stuck; I may propose a couple of things:

  • if you have a cleaner branch I can try to take over it (preferred)
  • restart from this PR
  • start again from scratch

Let me know if and how I can help!

@denistakeda
Copy link
Copy Markdown
Owner

Hello @fpischedda . Basically this PR is mostly finished work, the only problem is that it breaks tests. The reason for it is that we need to test in both Clojure and ClojureScript, but reagent doesn't provide make-reaction for Clojure.
It would be great if you can take over this. So I think that the way to proceed is:

  • Take this branch
  • Rebase to a latest master
  • Fix a problem with tests
  • Create a new PR

@denistakeda
Copy link
Copy Markdown
Owner

Also, I would like to have some more declarative way to work with rum, something like re-posh for reagent. But this is a plan for the future.

@fpischedda
Copy link
Copy Markdown

@denistakeda

Thanks for your quick reply and for clarifying what needs to be done. I'll get back to you as soon as I have something ready or if I will need some help :)

Regarding something like re-posh for rum/citrus, it would be great to have it some day! If I will get familiar enough with both project internals it would be something I will be excited to work on, let's see what happens with this topic first.

@fpischedda fpischedda mentioned this pull request Jun 24, 2020
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.

4 participants