Skip to content

fix(dao): HSTORE checking like ENUM#1458

Merged
janmeier merged 4 commits intosequelize:masterfrom
tomchentw:hotfix/hstore_checking_fn
Mar 7, 2014
Merged

fix(dao): HSTORE checking like ENUM#1458
janmeier merged 4 commits intosequelize:masterfrom
tomchentw:hotfix/hstore_checking_fn

Conversation

@tomchentw
Copy link
Copy Markdown
Contributor

This allows user can define HSTORE attribute directly.

define like this

var Task = sequelize.define('Task', {
  title: Sequelize.STRING,
  description: Sequelize.TEXT,
  deadline: Sequelize.HSTORE
})

instead of

deadline: {
  type: Sequelize.HSTORE
}

I think I have to write a test case to reflect this?

This allows user can define HSTORE attribute directly.
@janmeier
Copy link
Copy Markdown
Member

janmeier commented Mar 4, 2014

Looks good :)

Yes, a test would be very awesome. A good place to put it would probably be to add it in data-types.test.js. Would you mind adding a test for ENUM as well? A simple test that checks that .sync does not return any errors would be fine.

@mickhansen
Copy link
Copy Markdown
Contributor

Not a huge fan of matching against toString().
But i guess we're already doing that so...

@tomchentw
Copy link
Copy Markdown
Contributor Author

Thanks for the direction of @janmeier ! But I have to sleep now. Will push some tests later on.

@mickhansen, yeah, I think so.

To be honest, this is a huge project and you guys are doing really awesome. 👍

@tomchentw
Copy link
Copy Markdown
Contributor Author

@janmeier I cannot find a correct way to write test against this. Could you give me a hand?

I've also fix several bugs in the following commits (those are with tests).

@janmeier
Copy link
Copy Markdown
Member

janmeier commented Mar 7, 2014

@tomchentw Actually we already have tests for enums I just realized, so that's fine. And your test already uses both ways of declaring a HSTORE column. No more tests needed from my point of view.

I think this looks pretty ready to merge - @mickhansen

@mickhansen
Copy link
Copy Markdown
Contributor

@janmeier i've had 2 beers so i'll defer to your judgement.

janmeier added a commit that referenced this pull request Mar 7, 2014
@janmeier janmeier merged commit dd8782c into sequelize:master Mar 7, 2014
@janmeier
Copy link
Copy Markdown
Member

janmeier commented Mar 7, 2014

In that case it's a merge :)

@tomchentw
Copy link
Copy Markdown
Contributor Author

@mickhansen I like your comment! ha
@janmeier :) Thank you guys!

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.

3 participants