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

Snowflake does not explicitly support collation in the same way as some other databases #641

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

lbazetto
Copy link
Contributor

Snowflake does not explicitly support collation in the same way as some other databases.

Like it shows in create table syntax
And in collation specification

So this fix removes Snowflake from DIALECTS_WITH_CHARSET_AND_COLLATE

@WikiRik
Copy link
Member

WikiRik commented Jan 10, 2024

@lbazetto
Copy link
Contributor Author

Copy link

codecov bot commented Jan 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0bfd7d5) 99.43% compared to head (548e48f) 99.43%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #641   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files          12       12           
  Lines        1422     1422           
  Branches      257      257           
=======================================
  Hits         1414     1414           
  Misses          8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mmkal
Copy link
Contributor

mmkal commented Jan 17, 2024

@QuentinFarizon since you added this list, does this look ok to you?

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for charset and collate was indeed incorrectly added for snowflake in sequelize itself and will be removed in a next v7 alpha. That should also be reflected here

@QuentinFarizon
Copy link
Contributor

@mmkal sure, I wasn't the one suggesting adding it

@mmkal mmkal merged commit f4dfdd8 into sequelize:main Jan 17, 2024
Copy link

Released in v3.5.1.

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

Successfully merging this pull request may close these issues.

4 participants