Skip to content
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

make SQLite3 last_inserted_id private and organize DatabaseStatement methods #35922

Conversation

michaelglass
Copy link
Contributor

@michaelglass michaelglass commented Apr 10, 2019

Summary

This PR

  1. moves SQLite3 methods previously in the root adapter class into the Sqlite3::DatabaseStatement module if they're also in the Abstract::DatabaseStatement module.
  2. makes SQLIte3's last_inserted_id private

The inspiration for this change was that when upgrading our live_fixtures gem to rails 5, tests passed when running against sqlite3, but when consuming the gem in a mysql database, the tests failed. This was because last_inserted_id was a private method for mysql.

If the first part of the change is considered "cosmetic" I'm happy to revert it. I think it's a good idea to make last_inserted_id private, though.

…qlite3::DatabaseStatements and make those that are private in Abstract::DatabaseStatements private for sqlite3
@michaelglass michaelglass changed the title move SQLite3 DatabaseStatement methods into DatabaseStatement module make SQLite3 last_inserted_id private and organize DatabaseStatement methods Apr 10, 2019
@rafaelfranca rafaelfranca merged commit d8ab7ef into rails:master Apr 11, 2019
@michaelglass
Copy link
Contributor Author

Thank you!

@michaelglass michaelglass deleted the move-sqlite-3-database-statements-into-database-statements branch April 12, 2019 07:30
kamipo added a commit that referenced this pull request May 21, 2019
Almost all database statements methods except `explain` was moved into
`DatabaseStatements` at #35922. This moves the last one method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants