search

pinnymz / migration_comments

Comments for your migrations
MIT License
173 stars 31 forks source link

v0.4.0 introduces a breaking API change for setting table comments #35

Closed kenaniah closed 8 years ago

kenaniah commented 8 years ago

The breaking change was introduced in https://github.com/pinnymz/migration_comments/commit/a9646f937d2713707e2c1ddfe687432139393ac0#diff-24cef9304fab05a7c934b31754f74b8aL5 to accommodate the Rails 5 schema comments API.

This comment changes def comment(text) to def comment=(text), introducing a breaking change that is not mentioned in the ReadMe or elsewhere. Code samples in the Readme will also fail due to comment now being treated as an accessor in v0.4.0.

The Fix:

The Readme should be updated to reflect the changes in the API.

All instances of

t.comment "My table comment"

should be replaced with:

t.comment = "My table comment"
pinnymz commented 8 years ago

@kenaniah thanks for the report and the PR. However, are you sure the original behavior is broken? I believe that this test is covering this scenario and it's passing for me locally.

kenaniah commented 8 years ago

I discovered this issue when moving to the new version on one of my Rails 4.2 projects. Only change that was made was the gem's version. I can paste the stack trace at a later point if you'd like.

pinnymz commented 8 years ago

@kenaniah I'm not sure how to recreate this - I have Travis testing the latest Rails 4.2.6 and it's not complaining. A stack trace may help us here. Alternatively, can you come up with a test scenario that is failing for you?

kenaniah commented 8 years ago

Relevant code:

class CreateBasics < ActiveRecord::Migration
  def change
    create_table :privileges do |t|
      t.comment "Authorization privileges that are available to users"
      t.text :category
      t.text :item
      t.text :permission
  end
end

Output of rake db:migrate:

rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

wrong number of arguments (given 1, expected 0)
.../rails-project/db/migrate/1_create_basics.rb:25:in `block in change'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/migration_comments-0.4.1/lib/migration_comments/active_record/connection_adapters/postgresql_adapter.rb:43:in `block in create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/schema_statements.rb:216:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_plus_core-1.0.0/lib/schema_plus/core/active_record/connection_adapters/abstract_adapter.rb:28:in `block in create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:55:in `call_implementation'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:85:in `_modware_continue'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:77:in `_modware_call'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:44:in `execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:20:in `start'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_monkey-2.1.4/lib/schema_monkey/stack.rb:34:in `start'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_plus_core-1.0.0/lib/schema_plus/core/active_record/connection_adapters/abstract_adapter.rb:27:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/migration_comments-0.4.1/lib/migration_comments/active_record/connection_adapters/postgresql_adapter.rb:40:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:665:in `block in method_missing'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:634:in `block in say_with_time'
.../ruby.2.3.0/lib/ruby/2.3.0/benchmark.rb:293:in `measure'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:634:in `say_with_time'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:654:in `method_missing'
.../rails-project/db/migrate/1_create_basics.rb:24:in `change'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:608:in `exec_migration'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:592:in `block (2 levels) in migrate'
.../ruby.2.3.0/lib/ruby/2.3.0/benchmark.rb:293:in `measure'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:591:in `block in migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/connection_pool.rb:292:in `with_connection'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:590:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:768:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:998:in `block in execute_migration_in_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:1044:in `block in ddl_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `block in transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:184:in `within_new_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/transactions.rb:220:in `transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:1044:in `ddl_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:997:in `execute_migration_in_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:959:in `block in migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:955:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:955:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:823:in `up'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:801:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/tasks/database_tasks.rb:137:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/railties/databases.rake:44:in `block (2 levels) in <top (required)>'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:248:in `block in execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:243:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:243:in `execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:187:in `block in invoke_with_call_chain'
.../ruby.2.3.0/lib/ruby/2.3.0/monitor.rb:214:in `mon_synchronize'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:180:in `invoke_with_call_chain'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:173:in `invoke'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:152:in `invoke_task'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `block (2 levels) in top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `block in top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:117:in `run_with_threads'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:102:in `top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:80:in `block in run'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:178:in `standard_exception_handling'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:77:in `run'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/exe/rake:27:in `<top (required)>'
.../ruby.2.3.0/bin/rake:23:in `load'
.../ruby.2.3.0/bin/rake:23:in `<main>'
ArgumentError: wrong number of arguments (given 1, expected 0)
.../rails-project/db/migrate/1_create_basics.rb:25:in `block in change'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/migration_comments-0.4.1/lib/migration_comments/active_record/connection_adapters/postgresql_adapter.rb:43:in `block in create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/schema_statements.rb:216:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_plus_core-1.0.0/lib/schema_plus/core/active_record/connection_adapters/abstract_adapter.rb:28:in `block in create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:55:in `call_implementation'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:85:in `_modware_continue'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:77:in `_modware_call'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:44:in `execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/modware-0.1.2/lib/modware/stack.rb:20:in `start'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_monkey-2.1.4/lib/schema_monkey/stack.rb:34:in `start'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/schema_plus_core-1.0.0/lib/schema_plus/core/active_record/connection_adapters/abstract_adapter.rb:27:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/migration_comments-0.4.1/lib/migration_comments/active_record/connection_adapters/postgresql_adapter.rb:40:in `create_table'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:665:in `block in method_missing'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:634:in `block in say_with_time'
.../ruby.2.3.0/lib/ruby/2.3.0/benchmark.rb:293:in `measure'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:634:in `say_with_time'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:654:in `method_missing'
.../rails-project/db/migrate/1_create_basics.rb:24:in `change'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:608:in `exec_migration'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:592:in `block (2 levels) in migrate'
.../ruby.2.3.0/lib/ruby/2.3.0/benchmark.rb:293:in `measure'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:591:in `block in migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/connection_pool.rb:292:in `with_connection'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:590:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:768:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:998:in `block in execute_migration_in_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:1044:in `block in ddl_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `block in transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/transaction.rb:184:in `within_new_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:213:in `transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/transactions.rb:220:in `transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:1044:in `ddl_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:997:in `execute_migration_in_transaction'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:959:in `block in migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:955:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:955:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:823:in `up'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/migration.rb:801:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/tasks/database_tasks.rb:137:in `migrate'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/activerecord-4.2.6/lib/active_record/railties/databases.rake:44:in `block (2 levels) in <top (required)>'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:248:in `block in execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:243:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:243:in `execute'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:187:in `block in invoke_with_call_chain'
.../ruby.2.3.0/lib/ruby/2.3.0/monitor.rb:214:in `mon_synchronize'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:180:in `invoke_with_call_chain'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/task.rb:173:in `invoke'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:152:in `invoke_task'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `block (2 levels) in top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `each'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:108:in `block in top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:117:in `run_with_threads'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:102:in `top_level'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:80:in `block in run'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:178:in `standard_exception_handling'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/lib/rake/application.rb:77:in `run'
.../ruby.2.3.0/lib/ruby/gems/2.3.0/gems/rake-11.2.2/exe/rake:27:in `<top (required)>'
.../ruby.2.3.0/bin/rake:23:in `load'
.../ruby.2.3.0/bin/rake:23:in `<main>'
Tasks: TOP => db:migrate
pinnymz commented 8 years ago

Ok, I see the problem, and it's not with the gem :) You are using the create_table macro - and the t.comment syntax isn't supported for that. Instead you should pass that as an argument before the block:

create_table :privileges, comment: 'my comment' do |t|
  ... # columns/indexes
end

The t.comment syntax you referenced in the README (and still supported) is specifically for the change_table macro:

change_table :privileges do |t|
  t.comment 'my comment'
end

Please confirm if the alternate syntax works for you.

kenaniah commented 8 years ago

I see the gotcha now. That said, it would be great if the documentation and the gem mentioned one standard way to set the comment -- either using t.comment or t.comment=.

That said, there was a breaking API change, even if the original use case was not intended. Would it be possible to update the documentation to ensure others don't end up confused by this change as well?

pinnymz commented 8 years ago

Since the API as you were using it wasn't documented, I wouldn't consider it a breaking change. However, I feel your pain in having inconsistent interfaces. Unfortunately, this is mainly due to the Rails convention itself - the method signatures for table creation and table modification aren't a consistent match. The Rails 5 support for comments was implemented (not by myself) using the same interface as I have it here, and in all likelihood for the same reason.

So I have several choices here:

The first approach provides for the most elegant interface, but requires modifying the comment method signature which I consider a sort of hack. The second approach is inconsistent with the rest of the migration DSL, and won't be less confusing The third approach doesn't have either problem, but feels a bit more clunky. This opinion is subjective, of course. The last approach leaves us where we are, and frankly, I haven't had much complaints with this position :)

Given that this project is likely to sunset in the not-too-distant future (based on Rails 5 improvements), I'm inclined to not make any changes to the API as is. If there were a major push for this, I could see implementing the third approach as a viable solution, but I have no immediate plans for this.