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

feat(comment-property): allow column level comments (#4386) #9573

Conversation

mirkojotic
Copy link
Contributor

@mirkojotic mirkojotic commented Jun 21, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Re-introducing column level comments for mysql, postgres and mssql (sqlite doesn't support comments) #4386

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

Commit is mixed with changes from 7525ef0

Please just include your own changes

@codecov
Copy link

codecov bot commented Jun 21, 2018

Codecov Report

Merging #9573 into master will increase coverage by <.01%.
The diff coverage is 100%.

@mirkojotic
Copy link
Contributor Author

mirkojotic commented Jun 21, 2018

Removed code from another commit ( sorry about that ). Not sure what to do with mssql test suite failing. It doesn't seem like I've changed at that much and the error seems general. Would appreciate any and all feedback and suggestions.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

LGTM, just need one integration test

https://github.com/sequelize/sequelize/blob/master/test/integration/query-interface.test.js#L147

Modify this test add comment on one or more attributes, assert comment is properly available on all supported dialects as result of describeTable

Need docs in https://github.com/sequelize/sequelize/blob/master/docs/models-definition.md

On top you will see example of attribute definition, please add one more with comment property and a comment that this is only supported for Postgres, MySQL and MSSQL

@mirkojotic
Copy link
Contributor Author

mirkojotic commented Jun 23, 2018

@sushantdhiman I've added integration tests for mysql and postgres. In order to check for comment field I had to change describeTable statements for both dialects. I can change the mssql statement as well but my tests for mssql integration tests are all failing in docker environment and I've already spent some time exploring why. Since I'm running Ubuntu on my machine I was experiencing difficulties installing it locally.

I will work on documentation. Added an example and a note that it's only available in MySQL, PostgreSQL and MSSQL

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

LGTM, one comment and waiting for tests

@@ -78,7 +78,7 @@ class QueryGenerator {
})
);

return `DESCRIBE ${table};`;
return `SHOW FULL COLUMNS FROM ${table};`;
Copy link
Contributor

Choose a reason for hiding this comment

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

For MySQL? override this method in mysql/query-generator instead

@sushantdhiman sushantdhiman dismissed their stale review June 23, 2018 16:17

New changes as per review

@sushantdhiman
Copy link
Contributor

Since I'm running Ubuntu on my machine I was experiencing difficulties installing it locally.

If you are using docker, you can just

$ cd /to/sequelize/dev/folder
$ docker-compose up -d mssql

# I recommend .only to run only partial test suite as MSSQL image on Ubnutu fails for transactions
$ DIALECT=mssql npm run test-docker-integration

@mirkojotic
Copy link
Contributor Author

Still needs a bit more work. I'll try to finish it up later today.

@mirkojotic
Copy link
Contributor Author

@sushantdhiman hopefully this will do. Let me know if I need to make any corrections. Thank you for all the help and feedback!

@sushantdhiman sushantdhiman requested a review from a team June 25, 2018 05:20
@sushantdhiman sushantdhiman merged commit 9f2c9db into sequelize:master Jun 26, 2018
@sushantdhiman sushantdhiman mentioned this pull request Jun 26, 2018
@theRichu
Copy link

In migration process, addColumn and comment is not working

Postgres 10,

        return queryInterface.addColumn(
          {
            schema: 'project',
            tableName: 'scheduleAssigns'
          },
          'timeLogger',
          {
            type: Sequelize.JSONB,
            comment: 'Time Log on JSON',
            defaultValue: {}
          }
        )

Generated Query:

ALTER TABLE "project"."scheduleAssigns" ADD COLUMN "timeLogger" JSONB DEFAULT '{}' COMMENT Time Log on JSON;

I've got ERROR: syntax error at or near "COMMENT"

Should Be:

ALTER TABLE "project"."scheduleAssigns" ADD COLUMN "timeLogger" JSONB DEFAULT '{}'; COMMENT ON "project"."scheduleAssigns"."timeLogger" IS 'Time Log on JSON';

@mirkojotic
Copy link
Contributor Author

@theRichu PR you are referencing only adds comments as a part of create table statement. It will not work when modifying table definition. Could be added as a separate issue @sushantdhiman

@sushantdhiman
Copy link
Contributor

Now that @theRichu has opened a new PR #10076 we can discuss there

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.

None yet

4 participants