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

Update utils.js #275

Open
wants to merge 1 commit into
base: 0.12.x
Choose a base branch
from

Conversation

thephoenix-crypto
Copy link

When I was trying to use the ORM to create tables in postgreSQL with some column as 'varying character' datatype, I found that all string data types are converted into TEXT, even if I have given the size as specified in the documentation, the schema was not creating properly. So I made some changes in the case handling portion in utils.js file and it started working. If the size handling changes got added, it will be a great help for everyone. I even checked with the mysql library , there this particular issue is not present. Thanks.

When I was trying to use the ORM to create tables in postgreSQL with some column as 'varying character' datatype, I found that all string data types are converted into TEXT, even if I have given the size as specified in the documentation, the schema was not creating properly. So I made some changes in the case handling portion in utils.js file and it started working. If the size handling changes got added, it will be a great help for everyone. I even checked with the mysql library , there this particular issue is not present. Thanks.
@wulfsolter
Copy link

Tests?

@thephoenix-crypto
Copy link
Author

I didn't written a unit-test-case particular to this issue, but once after the file change the issue got resolved. And that check was done manually.

@mikermcneil
Copy link
Member

mikermcneil commented Apr 8, 2018

@mukesh-mohan So this is about adding a concept of autoMigrations.size, I think. I’m not sure I’d like to do that right now- the way we’ve been addressing this elsewhere is by using autoMigrations.columnType - conveniently aliased as columnType for compatibility (by either sails-hook-orm or waterline core, I believe).

In other words, I think this use case can be solved today by doing:

// Animal.js
{
  attributes: {
    status: {
      description: ‘A broad classification of this animal\’s status.,
      type: ‘string’,
      columnType: ‘CHARACTER VARYING(239),
      isIn: [‘healthy’, ‘deceased’, ‘ill’, ‘wounded’, ‘gestating’, ‘missing’],
      required: true
    },
  }
}

(columnType is a standard autoMigrations directive that is passed directly to the adapter.)

Copy link
Member

@mikermcneil mikermcneil left a comment

Choose a reason for hiding this comment

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

See #275 (comment) — I think we can get away without merging this, and still take care of this use case

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

Successfully merging this pull request may close these issues.

3 participants