Fork me on GitHub
#onyx
<
2018-01-24
>
niamu16:01:59

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

michaeldrogalis17:01:43

Thanks for the ping @niamu, I'll give it a final review now.

niamu17:01:08

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?

michaeldrogalis17:01:13

Can you change the PR to target master instead of 0.12.x?

michaeldrogalis17:01:24

Already created IIRC

michaeldrogalis17:01:56

Is L21 of myself.clj a repl left over?

niamu17:01:55

I don’t see that file in the list…

niamu17:01:18

oh, mysql.clj

michaeldrogalis17:01:26

Doh - sorry. Still coffee'ing

niamu17:01:01

No problem. No, that needs to be there to execute and modify the registered clauses in HoneySQL that it supports.

michaeldrogalis17:01:53

What is the significance of the 225?

niamu17:01:43

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

niamu17:01:23

There didn’t appear to be an equivalent MySQL library for HoneySQL, hence the reason for that new namespace in the pull request.

michaeldrogalis17:01:59

Cool -- can you tuck that constant into a def? Would be helpful to know why its important is all

michaeldrogalis17:01:03

Other than thats looks great to me

niamu17:01:45

Sure, I’ll make that change and include a link to that reference in the comment for future reviewers.

michaeldrogalis17:01:14

Excellent, thanks so much, and thanks for the contribution!

niamu17:01:27

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

michaeldrogalis18:01:52

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

michaeldrogalis18:01:27

@niamu Version 0.12.5.1-20180124.182029-2 has your patch

niamu18:01:35

Thanks so much!

niamu18:01:57

Now that it’s been merged, this issue can be closed as well: https://github.com/onyx-platform/onyx-sql/issues/17

michaeldrogalis18:01:12

Yep, closed. Thanks for that!