-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add support for finagle mysql #6
Conversation
…pull request goes live.
…inagle_mysql Conflicts: project.clj
…using them that I'm missing.
this looks pretty awesome! i'll take a closer look this weekend |
I have to figure out how to get it working with Travis MySQL; I've reviewed their docs and believe I'm using the right creds but I'll keep poking. |
(with-credentials "test" "test") | ||
(with-database "some_database") | ||
(rich-client "localhost:3306"))] | ||
(-> (prepare "SELECT * FROM widgets WHERE id = ? LIMIT 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't prepare have db
as its first argument?
This is really awesome Brian! a couple of small things to fixup besides getting this to run against mysql in CI. Not sure why it isn't working either, looks like you are doing the correct thing according to their docs. |
I think this is a limitation more generally of finagle-mysql, which seems Thanks for the feedback! Will push those fixes soon. On Saturday, January 10, 2015, Sam Neubardt notifications@github.com
|
could you add another root user with a password in the travis before_script? |
That's a great idea, I'll give it a shot. On Sunday, January 11, 2015, Sam Neubardt notifications@github.com wrote:
|
…yet another attempt at test passage.
Had any luck Brian? Want any help getting CI set up? |
also i'd be cool with removing the integration test and assuming the underlying framework works ok. |
I'd prefer to keep the tests in, but I'm just not sure how to diagnose this issue. I've tried reconfiguring the tests a few different ways, and apparently the user and database creation bits are working just fine. I'm not at all sure why they'd be okay but the table is nonetheless missing, and apparently there's no real way to replicate a Travis build locally. I'll keep trying. |
@samn VICTORY. The issue was case-sensitivity; it took a while to figure out because the stacktrace was entirely unhelpful. |
Awesome!! Thanks for not giving up on this. I'll give this a once over this weekend and likely do a new release with this. |
Nice! 🚢 |
a Clojure vector of maps" | ||
(->> (.rows rs) | ||
(scala/scala-seq->vec) | ||
(map Row->map))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this return a (lazy) seq, not a vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this to use mapv
and return a vector in f6a8971
This is so awesome. Thanks for your hard work on this @bguthrie! |
Submitted for a first look; feedback welcome.