-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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 stored procedure test in mysql2 #21932
Add stored procedure test in mysql2 #21932
Conversation
r? @sgrif (@rails-bot has picked a reviewer for you, use r? to override) |
# | ||
# In MySQL 5.6, CLIENT_MULTI_RESULTS is enabled by default. | ||
# http://dev.mysql.com/doc/refman/5.6/en/call.html | ||
if current_adapter?(:MysqlAdapter) && |
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.
You don't need to check the current adapter here.
Can you add a test for a procedure that returns more than one row? |
928f737
to
0ce9cfa
Compare
Thank you for your review. I added a test for a procedure that returns more than one row! |
DROP PROCEDURE IF EXISTS topics; | ||
SQL | ||
|
||
ActiveRecord::Base.connection.execute <<-SQL |
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.
Could these be merged into a single command?
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.
We need to enable CLIENT_MULTI_STATEMENTS
for multiple statements into a single command.
https://dev.mysql.com/doc/refman/5.6/en/c-api-multiple-queries.html
Should we enable the option for these queries?
A few more nitpicks. You can squash after they're addressed. |
0ce9cfa
to
049cd1a
Compare
049cd1a
to
d39b6f7
Compare
…sql2 Add stored procedure test in mysql2
No description provided.