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

allow for MySQL COMMENT field-attribute in Model attribute definition #523

Merged
merged 3 commits into from Jun 5, 2013

Conversation

2 participants
@iamjochem
Contributor

iamjochem commented Apr 8, 2013

make's it possible (when using MySQL) to define a comment property in a Model attribute's definition such that the DB Schema generated by calling the sync() method will include a COMMENT.

e.g.:

var User = sequelize.define('User', {
        username: {type: Sequelize.STRING, comment: 'Some lovely info for my DBA'}
});

// User.sync();

which would result in [roughly] the following schema SQL:

CREATE TABLE `User` IF NOT EXISTS
   `username` VARCHAR(255) COMMENT 'Some lovely info for my DBA', 
   `id` INTEGER NOT NULL auto_increment PRIMARY KEY
;
@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 8, 2013

bit of a fail (and yes I did read to be careful to not commit that config file :-/) - not that my local mysql password is any big secret, I only set it to something to make sure my local dev setup runs with a pwd set just like other env's.

It might be a good idea to be able to define an override config file for local testing (one that would be picked up in preference of the repo default if it exists AND is defined in .gitignore)?

Additionally the MySQL tests completely ignore the host and port settings ... which is not very handy if your local MySQL setup runs via a sockets rather than TCP.

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 9, 2013

Something looks weird here. The only diff is the test file, where is the actual code for this?

Travis does not report any errors, but if you look at the actual test run (https://travis-ci.org/sequelize/sequelize/jobs/6162367) your test fails - which makes sense since there is no code to support that feature ;)

I'll check up on whether this is just a single error in travis / jasmine or if travis doesnt actually fails when a jasmine test fails

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 9, 2013

Update on the testing issue:
It seems to be a general issue with our version of jasmine (1.0.17) that the exit code is always 0. Let's upgrade to 1.5 until we can completely migrate away from it

@iamjochem Is this a feature request that you just added tests for, or do you have the actual code somewhere :)? Also, could you please move the tests to buster (the /spec folder)

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Apr 9, 2013

@janmeier - er yeah, there should have been some code to go with the test, now included! ... it's only for MySQL because:

  • postgres does not support defining a comment when defining a field (it requires a seperate COMMENT query) .. I know nothing about postgres and only a little more about sequelize - so it's beyond me to implement this for postgres
  • sqlite does not support comments at all (although it does preserve comments in the actual SQL DDL ... so that might be something to consider ... but I'm not sure whether sequelize and/or the underlying sqlite lib support inline SQL comments in [raw] queries)

with regard to 'moving' the tests to buster ... no, I have no idea what the buster syntax is and there are no tests currently for the mysql connector's dao-factory (or any other dao-factory for that matter) that I can use as a reference for rewriting the jasmine test ... sorry. If you can help point me in the right direction in this regard I might be persuaded to have a go at rewriting the test (to be honest though given the number of tests that need to be ported to buster it seems a better idea to tackle that in a seperate PR).

@janmeier janmeier merged commit 0897e9c into sequelize:master Jun 5, 2013

1 check passed

default The Travis build passed
Details
@janmeier

This comment has been minimized.

Member

janmeier commented Jun 5, 2013

Thanks a bunch, I merged and added the same functionality for PG ;-)

You are right about the tests, no query generator tests in buster (yet!)

@iamjochem

This comment has been minimized.

Contributor

iamjochem commented Jun 6, 2013

cool! - glad that not having the buster tests in this case wasn't a deal breaker.

@durango durango referenced this pull request Jul 8, 2013

Closed

State of the Union #749

33 of 45 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment