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

Convert to cljc and reader conditionals #59

Merged
merged 6 commits into from
Jul 21, 2015
Merged

Convert to cljc and reader conditionals #59

merged 6 commits into from
Jul 21, 2015

Conversation

danielcompton
Copy link
Collaborator

New attempt, based on #26.

@danielytics
Copy link
Contributor

So am I correct in saying that the goal here is that rethinkdb.query is available from cljs so that the queries can be constructed in cljs (but actually sending the queries and receiving results is clj only, so only query.clj and query_builder.clj need to be ported)?

I would suggest maybe adding a build function to rethinkdb.query that calls replace-vars and then modifying run to optionally take a prebuilt query.

@danielcompton
Copy link
Collaborator Author

I need to check this, but I'm pretty sure we won't need to modify run at all. Calling (-> (r/db) (r/table :users)) will generate EDN data. That can be shipped across to the server where it calls (r/run client-query conn).

@danielcompton
Copy link
Collaborator Author

Regex's aren't being used anymore on the shared path after b1433fa.

danielcompton added a commit that referenced this pull request Jul 21, 2015
Convert to cljc and reader conditionals
@danielcompton danielcompton merged commit 979b81f into master Jul 21, 2015
@danielcompton
Copy link
Collaborator Author

I've merged this into master. There are however a few caveats:

  • fn isn't supported in CLJS
  • order-by isn't supported in CLJS

I'll open tickets to fix these separately.

@danielytics
Copy link
Contributor

I suppose it depends on if you want to call replace-vars as part of the AST building, or as part of running (on the server, later). If you're happy with the latter, then it works as is.

Great work, btw.

@danielcompton
Copy link
Collaborator Author

I haven't really looked at fn, what's the difference between doing it on the client or server?

@danielytics
Copy link
Contributor

Nothing much, its just a question of do you want to prepare the query as much as possible (ie the AST is as baked and prepared as possible) or if you just care that the representation is captured.

If you don't want to, say, store the most processed form of a query ready to send to the RethinkDB server and only care that you can create queries from cljs (and store them or whatever), then no changes are necessary.

All replace-vars does is make sure that unique integer ID's are assigned to variables that shadow others of the same name in nested scopes.

I suspect the answer is that you just want to be able to generate the query in cljs but don't care that (slightly) more processing is needed before running the query :) so I don't think anything needs to be done. I just wanted to check to be sure.

@danielcompton
Copy link
Collaborator Author

Right, makes sense. Probably just as easy to leave it to be done on the server, it should have an added benefit of being slightly easier to debug if we have variable names in them.

@danielytics
Copy link
Contributor

Agreed. Like I said, I just wanted to make sure it was thought about. I don't mind which way you choose, but you're right that doing it on the server would be better for debugging, so lets do that.

@arichiardi
Copy link
Contributor

Hello guys, sorry if I write in a closed issue, but how to start with ClojureScript? I tried [rethinkdb.query :as r :include-macros true] but I saw there is an IllegalArgumentException no in a clj block in there. Shall I patch it? Maybe the question is more like, what is this namespace missing in terms of reader conditionals?

@apa512
Copy link
Owner

apa512 commented May 8, 2016

A patch would be great. IllegalArgumentException is probably the only thing that has to be changed. I totally forgot about ClojureScript when adding that.

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

Successfully merging this pull request may close these issues.

None yet

4 participants