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

Remove unused pk_and_sequence_for in AbstractMysqlAdapter #21686

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Sep 20, 2015

pk_and_sequence_for is implemented for PG and MySQL adapters (not
implemented for Sqlite3 adapter). But MySQL adapters are not using
pk_and_sequence_for already. We can remove the method.

@rails-bot
Copy link

r? @senny

(@rails-bot has picked a reviewer for you, use r? to override)

@pixeltrix
Copy link
Contributor

This method appears in the documentation (which may have been accidental) so we'd need to deprecate it first before we remove it.

@jeremy
Copy link
Member

jeremy commented Sep 20, 2015

+Deprecation, but that only covers callers of this method. I'd guess that most usage is people overriding/patching the method to change its behavior, and that will no longer work anyway. Removing the method would cause the overrides to fail, which may be easier to understand than a deprecation.

https://github.com/search?l=ruby&q=pk_and_sequence_for&type=Code&utf8=✓

@senny
Copy link
Member

senny commented Sep 22, 2015

r? @jeremy

@rails-bot rails-bot assigned jeremy and unassigned senny Sep 22, 2015
@arthurnn
Copy link
Member

Agree with @jeremy . We can remove it, if we dont use it.

@kamipo can you rebase this branch and I will merge it.
thanks

@kamipo
Copy link
Member Author

kamipo commented Sep 28, 2015

@arthurnn I rebased it, thanks!

@arthurnn
Copy link
Member

Also, can you add a note on the Changelog for it. as it was part of the Docs, and we are removing it without deprecation, I would say it is nice to have it in the logs.

`pk_and_sequence_for` is implemented for PG and MySQL adapters (not
implemented for Sqlite3 adapter). But MySQL adapters are not using
`pk_and_sequence_for` already.
@kamipo
Copy link
Member Author

kamipo commented Oct 8, 2015

I added to the Changelog.

pixeltrix added a commit that referenced this pull request Oct 8, 2015
Remove unused `pk_and_sequence_for` in AbstractMysqlAdapter
@pixeltrix pixeltrix merged commit c750ca5 into rails:master Oct 8, 2015
@pixeltrix
Copy link
Contributor

@kamipo thanks! 👍

@kamipo
Copy link
Member Author

kamipo commented Oct 8, 2015

Thank you for quick merging 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants