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 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

This comment has been minimized.

Copy link

rails-bot commented Sep 20, 2015

r? @senny

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

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Sep 20, 2015

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

@jeremy

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

senny commented Sep 22, 2015

r? @jeremy

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

This comment has been minimized.

Copy link
Member

arthurnn commented Sep 28, 2015

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 kamipo force-pushed the kamipo:remove_pk_and_sequence_for branch from ab31620 to 5e8f502 Sep 28, 2015
@kamipo

This comment has been minimized.

Copy link
Member Author

kamipo commented Sep 28, 2015

@arthurnn I rebased it, thanks!

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Sep 28, 2015

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 kamipo force-pushed the kamipo:remove_pk_and_sequence_for branch from 5e8f502 to fd37486 Oct 7, 2015
@kamipo

This comment has been minimized.

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
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Oct 8, 2015

@kamipo thanks! 👍

@kamipo

This comment has been minimized.

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
Projects
None yet
6 participants
You can’t perform that action at this time.