This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2018-01-24
Channels
- # arachne (3)
- # beginners (39)
- # boot (3)
- # cider (91)
- # cljs-dev (56)
- # cljsrn (4)
- # clojure (267)
- # clojure-dusseldorf (1)
- # clojure-estonia (1)
- # clojure-greece (2)
- # clojure-italy (6)
- # clojure-nl (2)
- # clojure-russia (18)
- # clojure-spec (27)
- # clojure-uk (136)
- # clojurescript (19)
- # core-async (2)
- # cursive (6)
- # datomic (17)
- # emacs (2)
- # fulcro (86)
- # graphql (4)
- # hoplon (13)
- # jobs-discuss (7)
- # jobs-rus (1)
- # keechma (34)
- # keyboards (7)
- # leiningen (5)
- # luminus (4)
- # lumo (8)
- # off-topic (13)
- # om (6)
- # onyx (26)
- # re-frame (22)
- # reagent (1)
- # reitit (2)
- # remote-jobs (8)
- # ring (3)
- # ring-swagger (5)
- # rum (8)
- # shadow-cljs (45)
- # specter (6)
- # unrepl (16)
- # yada (15)
I don’t mean to be a pest, but I think this pull request is pretty solid at this point. If anyone has time to look at it, let me know what else needs to be addressed: https://github.com/onyx-platform/onyx-sql/pull/31
Thanks for the ping @niamu, I'll give it a final review now.
On an unrelated note, when defining the bucket to use for :onyx.peer/storage.s3.bucket
, does it need to be created already or will it be created on demand?
Can you change the PR to target master instead of 0.12.x?
Already created IIRC
Is L21 of myself.clj
a repl left over?
https://github.com/onyx-platform/onyx-sql/pull/31/files#diff-bd990a4bb3e18602cf74093067b8ffc6
Doh - sorry. Still coffee'ing
No problem. No, that needs to be there to execute and modify the registered clauses in HoneySQL that it supports.
What is the significance of the 225
?
It’s the order number of how to construct parts of the query string. In this case it will be inserted towards the end of the query. I used honeysql-postgres as a reference for the number: https://github.com/nilenso/honeysql-postgres/blob/932ca5113568407c3c9df480cfd0949fddc8e4ab/src/honeysql_postgres/format.clj#L19
There didn’t appear to be an equivalent MySQL library for HoneySQL, hence the reason for that new namespace in the pull request.
Cool -- can you tuck that constant into a def? Would be helpful to know why its important is all
Other than thats looks great to me
Sure, I’ll make that change and include a link to that reference in the comment for future reviewers.
Excellent, thanks so much, and thanks for the contribution!
@niamu Merged, thanks! CI will cut a release now, stay tuned.
@niamu Version 0.12.5.1-20180124.182029-2
has your patch
No problem!
Now that it’s been merged, this issue can be closed as well: https://github.com/onyx-platform/onyx-sql/issues/17
Yep, closed. Thanks for that!