Clojurians Log v2

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?

will do

Already created IIRC

Is L21 of myself.clj a repl left over?

I don’t see that file in the list…

https://github.com/onyx-platform/onyx-sql/pull/31/files#diff-bd990a4bb3e18602cf74093067b8ffc6

oh, mysql.clj

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!

Alright. Updated with a def and a comment to explain what that number does.

@niamu Merged, thanks! CI will cut a release now, stay tuned.

@niamu Version 0.12.5.1-20180124.182029-2 has your patch

Thanks so much!

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!