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

Dump column collation to schema.rb and allow collation changes using column_change #881

Conversation

mgrunberg
Copy link
Contributor

This PR address #848.

It also adds support to change collation change_column :customers, :name, collation: :xxx. Before this change, collation changes were skipped/ignored.

@mgrunberg
Copy link
Contributor Author

I decided to implement this on 6-0-stable since it's a small change. If it wasn't the right call and it should be targeting main, let me know.

@mgrunberg
Copy link
Contributor Author

I've just realized that the issue was reported for 5-2-stable.

The fix is almost the same. The only difference with this PR is that the change in lib/active_record/connection_adapters/sqlserver/schema_statements.rb should be

sql_commands.last << " COLLATE #{options[:collation]}" if options[:collation].present?

and not

alter_command += " COLLATE #{options[:collation]}" if options[:collation].present?

I'm not sure what's the best here. I can make another PR for 5-2-stable.

@wpolicarpo
Copy link
Member

PRs should target the main branch. We can backport to stable branches if necessary. Can you rebase your branch and change the target to main?

@mgrunberg mgrunberg force-pushed the issues/yellowspot/848_fix_schema_column_collection branch from 0dd6e80 to d367bda Compare April 19, 2021 13:44
@mgrunberg mgrunberg changed the base branch from 6-0-stable to main April 19, 2021 13:45
@mgrunberg
Copy link
Contributor Author

mgrunberg commented Apr 19, 2021

@wpolicarpo, no problem. I've rebased with main and changed target to main.

@wpolicarpo
Copy link
Member

LGTM. Can you resolve the changelog conflict?

@mgrunberg
Copy link
Contributor Author

@wpolicarpo, conflicts fixed

@wpolicarpo wpolicarpo merged commit c98ea79 into rails-sqlserver:main Apr 19, 2021
@mgrunberg mgrunberg deleted the issues/yellowspot/848_fix_schema_column_collection branch January 3, 2022 17:33
lavika pushed a commit to lavika/activerecord-sqlserver-adapter that referenced this pull request Sep 26, 2023
…column_change (rails-sqlserver#881)

* add column collation to schema dump. Dump string value

* be able to alter column collation

* spec showing collation change using 'change_column'

* update changelog
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

2 participants