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

WIP: Add support for specifying default comment as the name of the migration file #24622

Closed

Conversation

vipulnsward
Copy link
Member

Based on #22911 (comment)

Opening for comments/discussion

@vipulnsward
Copy link
Member Author

@jeremy something like this adds support for create_table/index/column.
Do you think adding this to t.column makes sense, or it can get cluttered.

@vipulnsward
Copy link
Member Author

Example:

class CreateContainers < ActiveRecord::Migration[5.0]
  def change
    create_table :containers, comment: 'This is from containers' do |t|
      t.string :name, comment: 'Does this work?'
      t.integer :name_without_comment
      t.timestamps
    end
    add_index :containers, :name_without_comment
  end
end

to

CREATE TABLE `containers` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `name` varchar(255) DEFAULT NULL COMMENT 'Does this work?',
  `name_without_comment` int(11) DEFAULT NULL,
  `created_at` datetime NOT NULL,
  `updated_at` datetime NOT NULL,
  PRIMARY KEY (`id`),
  KEY `index_containers_on_name_without_comment` (`name_without_comment`) COMMENT '20160416155605_create_containers.rb'
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='This is from containers';

@vipulnsward
Copy link
Member Author

To think about it, @dhh is not going to like this as it will clutter the schema.rb . Perhaps an config to enable/disable it maybe. Anyway, waiting for confirmation, if we need this.

@dhh
Copy link
Member

dhh commented Apr 19, 2016

Sounds good to me. Would be captured in schema.rb as well, yeah?

@vipulnsward
Copy link
Member Author

Yes, it will be captured there. Example: https://gist.github.com/vipulnsward/653c6c1f8d3636c5900c0823d9ce03bb

@dhh
Copy link
Member

dhh commented Apr 19, 2016

Ah, I see the question. So the litter would be if we automatically marked
every single row with which migration it came from? Maybe simulate that and
see how it feels. I do suspect its going to feel a bit busy AND perhaps
drown out the manual comments too.

On Tue, Apr 19, 2016 at 10:05 AM, Vipul A M notifications@github.com
wrote:

Yes, it will be captured there. Example:
https://gist.github.com/vipulnsward/653c6c1f8d3636c5900c0823d9ce03bb


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24622 (comment)

@maclover7
Copy link
Contributor

I'm personally 👎 on this as a default -- I feel like it adds a lot of noise to migrations / schema.rb, and isn't really super useful out of the box to end users -- I think this would be a neat opt-in feature though :)

@jeremy
Copy link
Member

jeremy commented Apr 20, 2016

I tried it out with Basecamp's schema.rb and it's pretty neat but does feel like it crosses the noise line. It's nice when you want a high-level view of what's affected your schema, but git blame does a decent job of that today. Fun exploration in any case! Thank you @vipulnsward ❤️

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

Successfully merging this pull request may close these issues.

None yet

4 participants