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

fix: describe query should not uppercase the enum type #5321

Merged
merged 1 commit into from Feb 15, 2016

Conversation

2 participants
@luin
Contributor

luin commented Jan 28, 2016

Our team relies on the result of describeTable method to generate migrations automatically. However, we found that enum types are uppercased by sequellize by mistake.

For example, we have a field with the type of enum('admin', 'member', 'guest'), the result of describeTable will be ENUM('ADMIN', 'MEMBER', 'GUEST'), which is obviously not as same as the former one.

After the changes, the result will become ENUM('admin', 'member', 'guest').

@luin luin force-pushed the luin:fix-uppercase-enum branch from b6c27c4 to 33a8fde Jan 28, 2016

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 28, 2016

LGTM, just needs a test

@janmeier janmeier self-assigned this Jan 28, 2016

@luin

This comment has been minimized.

Contributor

luin commented Jan 28, 2016

Yeah. Since I'm not familiar with the code base of sequelize, would you mind to write a test for this? Just did a quick search and it seems formatResults function is not test-covered yet.

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 30, 2016

You can add an integration test, or alter one of the existing ones to include an enum https://github.com/sequelize/sequelize/blob/master/test/integration/query-interface.test.js#L148

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 10, 2016

pING @luin

@luin

This comment has been minimized.

Contributor

luin commented Feb 10, 2016

A little busy these days. I'll write tests for it soon.

@luin luin force-pushed the luin:fix-uppercase-enum branch from 62c6655 to 09893b3 Feb 13, 2016

@luin

This comment has been minimized.

Contributor

luin commented Feb 13, 2016

added and rebased

janmeier added a commit that referenced this pull request Feb 15, 2016

Merge pull request #5321 from luin/fix-uppercase-enum
fix: describe query should not uppercase the enum type

@janmeier janmeier merged commit b48f065 into sequelize:master Feb 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

janmeier added a commit that referenced this pull request Feb 15, 2016

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 15, 2016

Thanks! :)

@luin luin deleted the luin:fix-uppercase-enum branch Feb 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment