Skip to content

Generating proper migration when ActiveRecord::Base.pluralize_table_names = false #13440

Merged
merged 1 commit into from Mar 25, 2014

5 participants

@kuldeepaggarwal

Earlier, when pluralize_table_names = false, then if we try to create new migration then it does not consider the ActiveRecord::Base.pluralize_table_names configuration and always pluaralizing the table name.

Before
rails g migration add_column_test1_to_user test1
# => 
class AddColumnTest1ToUser < ActiveRecord::Migration
  def change
    add_column :users, :test1, :string
  end
end


Now:
rails g migration add_column_test1_to_user test1
# =>
class AddColumnTest1ToUser < ActiveRecord::Migration
  def change
    add_column :user, :test1, :string
  end
end

fixes #13426

@robin850 robin850 and 1 other commented on an outdated diff Dec 21, 2013
...rators/active_record/migration/migration_generator.rb
@@ -61,6 +61,14 @@ def validate_file_name!
raise IllegalMigrationNameError.new(file_name)
end
end
+
+ def pluralize_table_names?
@robin850
Ruby on Rails member
robin850 added a note Dec 21, 2013

Actually I'm wondering why this method and plural_table_name are only available in the NamedBase generator and not the Base one. With this patch, we would have to duplicate these methods elsewhere in case we need to know whether the option is set to true or not.

Let's wait for some feedback. I haven't found anything legacy reason in the logs.

We don't need to copy this method as it is already inherited from NamedBase so we can access all methods of NamedBase class. Removed the duplicate code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robin850 robin850 and 1 other commented on an outdated diff Dec 21, 2013
railties/test/generators/migration_generator_test.rb
+ assert_method :change, content do |change|
+ assert_match(/create_table :book/, change)
+ assert_match(/ t\.string :title/, change)
+ assert_match(/ t\.text :content/, change)
+ end
+ end
+ end
+ end
+
+ private
+
+ def with_singular_table_name
+ old_state = ActiveRecord::Base.pluralize_table_names
+ ActiveRecord::Base.pluralize_table_names = false
+ yield
+ ActiveRecord::Base.pluralize_table_names = old_state
@robin850
Ruby on Rails member
robin850 added a note Dec 21, 2013

What do you think about using ensure to make sure that the previous state would be correctly set in case the test fails ?

Yeah you are right, I was also thinking that I am missing something. I will update it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@robin850
Ruby on Rails member

Thanks @kuldeepaggarwal. Could you please add a changelog entry ?

@kuldeepaggarwal

@chancancode Please review this too.

@chancancode
Ruby on Rails member

I am not particularly knowledgable in this area, so I'll defer to others :)

By the way, if you are interested, there is one thing that bugs me a lot and I would love to see fixed. Last time I check, if you pass the "wrong" pluralization to the scaffold and resource generator, it will warn you about it and make you confirm it by passing --plural or something. However this is not true for the model generator.

Everytime I teach rails classes, I always see some students falling into this trap, and I often had to recommend them tearing everything down with rails destroy model wrong_name to start over. Heck, I even do that all the time myself.

If, as part of your work here (or whoever else would like to pick this up), you can refactor the related stuff, you'll be a hero to many newbies :grin:

Not saying you need to do it as part of this, just throwing it out there in case you want to help :)

@kuldeepaggarwal

@chancancode Can you please provide a gist to understand the actual problem? I am not able to get it.

@chancancode
Ruby on Rails member

@kuldeepaggarwal Sure, I showed it in #6809, probably one of the first rails issues I submitted :P

@kuldeepaggarwal

@chancancode I have fixed the issue which you told me above. Now rails g model wrong_name will create correct model(wrong_name.singularize) and correct corresponding files. I will create PR asap.

@chancancode
Ruby on Rails member

:heart:

@arunagw
Ruby on Rails member
arunagw commented Dec 28, 2013

I can see that rebase is required with master and also need to squash commits.

@kuldeepaggarwal

@arunagw Actually second commit is not related to any opened issue but the former one is, so that's why I made 2 different commits. Should I still squash them?

@arunagw
Ruby on Rails member
arunagw commented Dec 28, 2013

Yes please! If commits are related to each other better to squash as they are easy to revert in future if required :smile:

If these are not related to each other you can do another PR for second commit.

@kuldeepaggarwal

@arunagw They are not related to each other, I will make another PR for the second commit.

@kuldeepaggarwal

@arunagw Rebased with master and removed the second commit(will create another PR).

@kuldeepaggarwal

any updates on this?

@kuldeepaggarwal

any updates on this?

@kuldeepaggarwal

@robin850 Updated my PR.

@robin850 robin850 added the railties label Feb 23, 2014
@senny senny and 1 other commented on an outdated diff Feb 25, 2014
...rators/active_record/migration/migration_generator.rb
@@ -61,6 +61,10 @@ def validate_file_name!
raise IllegalMigrationNameError.new(file_name)
end
end
+
+ def get_table_name(_table_name)
@senny
Ruby on Rails member
senny added a note Feb 25, 2014

I don't like the get_ prefix. Maybe use normalize_?

I think table_name would be better. wdyt?

@senny
Ruby on Rails member
senny added a note Feb 25, 2014

I'd rather have a name that signals some sort of calculation is happening.

Yeah, you are right.. Will rename the method to normalize_table_name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kuldeepaggarwal

@senny Updated PR

@kuldeepaggarwal

@senny All tests are green :smile:

@kuldeepaggarwal

@senny any updates on this?

@senny
Ruby on Rails member
senny commented Feb 27, 2014

@kuldeepaggarwal I have some other issues pending with higher priority. Will come back once I got them finished.

@kuldeepaggarwal

@senny any updates?

@senny senny commented on the diff Mar 25, 2014
activerecord/CHANGELOG.md
@@ -1,3 +1,14 @@
+* Fix Generation of proper migration when
+ ActiveRecord::Base.pluralize_table_names = false.
@senny
Ruby on Rails member
senny added a note Mar 25, 2014

Can you add Fixes #13426.?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny senny commented on an outdated diff Mar 25, 2014
railties/test/generators/migration_generator_test.rb
+ run_generator ["create_book", "title:string", "content:text"]
+ assert_migration "db/migrate/create_book.rb" do |content|
+ assert_method :change, content do |change|
+ assert_match(/create_table :book/, change)
+ assert_match(/ t\.string :title/, change)
+ assert_match(/ t\.text :content/, change)
+ end
+ end
+ end
+ end
+
+ private
+
+ def with_singular_table_name
+ old_state = ActiveRecord::Base.pluralize_table_names
+ begin
@senny
Ruby on Rails member
senny added a note Mar 25, 2014

no need to begin here. You can use the default ensure cause of the method:

def with_singular_table_name
  old_state = ActiveRecord::Base.pluralize_table_names
  ActiveRecord::Base.pluralize_table_names = false
  yield
ensure
  ActiveRecord::Base.pluralize_table_names = old_state
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@senny
Ruby on Rails member
senny commented Mar 25, 2014

@kuldeepaggarwal there are a lot of tasks on my plate. Asking every other week won't help to resolve them faster...

The patch is looking good. I added a couple minor comments. We also need a rebase. Thank you for your work :yellow_heart:

@kuldeepaggarwal

@senny Sorry for creating noise on the PR. :cry:

@kuldeepaggarwal

@senny Updated PR.

@senny senny commented on an outdated diff Mar 25, 2014
activerecord/CHANGELOG.md
@@ -1,3 +1,15 @@
+* Fixes #13426.
@senny
Ruby on Rails member
senny added a note Mar 25, 2014

Fixes goes at the end of the CHANGELOG. See other entries for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kuldeepaggarwal kuldeepaggarwal Fix Generation of proper migration when
  ActiveRecord::Base.pluralize_table_names = false.

  Previously, generation a migration like this:

      rails g migration add_column_name_to_user name

  would not generating the correct table name.

Fixes #13426.
5a3817c
@kuldeepaggarwal

@senny Sorry didn't know that, updated the CHANGELOG. :smile:

@senny senny merged commit c35fb52 into rails:master Mar 25, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.