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

Allow column comments #558

Open
1 of 3 tasks
Nosfistis opened this issue Mar 21, 2018 · 12 comments
Open
1 of 3 tasks

Allow column comments #558

Nosfistis opened this issue Mar 21, 2018 · 12 comments

Comments

@Nosfistis
Copy link

Nosfistis commented Mar 21, 2018

I'm submitting a...

  • Bug report
  • Feature request
  • Question

I have not found a way to support comments per column (working on MySQL). I might be missing something, but if not already supported, can it be added?


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@stale
Copy link

stale bot commented Apr 20, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@wzrdtales
Copy link
Member

wzrdtales commented Apr 27, 2018

Column Comments are completely niche, nearly no one uses them, so it is not really high on prio. However if you want to submit a PR for it, you're welcome.

@Nosfistis
Copy link
Author

Implementing this, I should add an entry to both node-db-migrate's interfaces as well as an implementation in the mysql driver, right?

Assuming that a comment can be either a table or a column comment, adding comment would be something like:

exports.up = function (db, callback) {
  db.createTable('pets', {
    columns: {
      id: { type: 'int', primaryKey: true, autoIncrement: true, comment: 'primary key' },
      name: 'string'  // shorthand notation
    },
    comment: 'pets table stores all pets',
    ifNotExists: true
  }, callback);
}

@wzrdtales
Copy link
Member

No just the driver, since this is mostly MySQL exclusive stuff, most other dbs don't have that feature. But anyway, no there is no need to touch the interface, rather it would be an extension of alterColumn and createTable.

Your provided example however is ok. The shorthand notation could make sense, but I would put that outside of the scope of this addition and discuss that separately.

You will also need to move table up to the mysql driver directly

https://github.com/db-migrate/db-migrate-base/blob/7af3f80bad7fa5c64509a4435cce68891403c0d1/index.js#L172-L245

And after that you just need to handle comment accordingly.

@Nosfistis
Copy link
Author

The sorthand notation is not part of my proposal (I'm sorry for the confusion), rather part of an example taken from the docs.

If I understand your suggestion correctly, that means the createTable logic? Maybe an altering clause after createTable finishes is cleaner:
ALTER TABLE MyTable COMMENT = 'happy comment';

@wzrdtales
Copy link
Member

No please keep operations atomic.

@wzrdtales
Copy link
Member

wzrdtales commented May 10, 2018

Either it is one create table or you must pass two statements also in db-migrate.

@machinshin
Copy link

actually, postgres also has this functionality. But, since it's not SQL standard, their interface is very different than mySQL's => https://www.postgresql.org/docs/10/static/sql-comment.html

@Nosfistis
Copy link
Author

I will be going forth with the separate table function to the mysql driver. If another driver such as postgres needs this functionality I guess that would need another implementation, since the common interface supports more db's which do not have implementations (yet).

@Nosfistis
Copy link
Author

It seems that my table comment (comment associated with the table directly and not with a column) conflicts with the quick column definition (trying to create a column named "comment" of type whatever the comment text is).

In order to make the above notation work, comment has to become a reserved word in mysql and if a column named "comment" is needed the extended notation is required.

@wzrdtales
Copy link
Member

Currently low on, time will try to respond in the next days. But concerning the comment reserved keyword, this is a no go and not gonna happen. actually when creating with the columns notation this should however alreay work. See the logic for this here:

https://github.com/db-migrate/db-migrate-base/blob/7af3f80bad7fa5c64509a4435cce68891403c0d1/index.js#L177-L181

@Nosfistis
Copy link
Author

I have created the adjustment for column comments but table comments is still not agreed upon. Any input?

The current implementation works with changeColumn and any column spec since it simply enhances it.
I shall try to add some tests for that as well.

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

No branches or pull requests

3 participants