Model generator should print a warning if passed a plural name and automatically convert it #6809

Closed
chancancode opened this Issue Jun 21, 2012 · 7 comments

Comments

Projects
None yet
4 participants
@chancancode
Member

chancancode commented Jun 21, 2012

I'm sure almost every single Rails developer out there have made this mistake at some point:

$ rails g model posts title:string
      invoke  active_record
      create    db/migrate/20120621124605_create_posts.rb
      create    app/models/posts.rb
      invoke    test_unit
      create      test/unit/posts_test.rb
      create      test/fixtures/posts.yml

If you didn't notice the problem, you may skip the rest of this. Simply go ahead and 👍 this ;)

"Obviously the name of a model should be singular! Everyone knows that."

Maybe. (Personally speaking, I can never remember what the generators expect.) But people make mistakes (case in point - #6746). I volunteered for RailsBridge and introduced plenty of new developers to Rails, and based on what I've seen this is not an uncommon cause of confusion and frustration for beginners. ("Won't somebody please think of the newbies?")

If you are still not convinced, our @tenderlove has famously made this mistake and was caught on camera :P

LOL

Since we are already auto-correcting this for resource and scaffold generators, I don't see why we shouldn't do this on the model generator too.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jun 21, 2012

Member

I was going to submit this as a PR, but after some digging around I realized the problem is more complicated than I thought:

  1. Previously, String#singularize is not idempotent. The classic example is 'address' => 'addres'. This bug was fixed in #4719, so this should no longer be a problem.
  2. I'm not sure how NamedBase is supposed to work. From what I understand, the "name" parameter is assumed to be singular. I traced all the way back to https://github.com/rails/rails/blob/83f7fe2028df15e058205c93308c0e19abd7157a/railties/lib/generator/named_base.rb#L56-57, and it appears that the plural name is always pluralized explicitly while the singular name is more or less directly taken from the name parameter. (I am guessing this might have something to do with the bug I mentioned earlier. @josevalim?) This seems wrong to me as it is clearly not how it is being used now (e.g. the resource generator passes in a plural name here when creating the controllers). Even if it's not wrong the naming makes it extremely confusing.
  3. I'm not sure where I should implement the check. (NamedBase is probably not a good place to put that logic as it will probably break a lot of generators out there.) Perhaps I should move this to railties/lib/rails/generators/rails/model/model_generator.rb? Would that work?

I really want this bug fixed (if this get fixed, it'll be my favourite new feature in Rails 4!), and if someone can clarify things for me / provide some suggestions, I'm willing to spend some time coming up with a solution.

Member

chancancode commented Jun 21, 2012

I was going to submit this as a PR, but after some digging around I realized the problem is more complicated than I thought:

  1. Previously, String#singularize is not idempotent. The classic example is 'address' => 'addres'. This bug was fixed in #4719, so this should no longer be a problem.
  2. I'm not sure how NamedBase is supposed to work. From what I understand, the "name" parameter is assumed to be singular. I traced all the way back to https://github.com/rails/rails/blob/83f7fe2028df15e058205c93308c0e19abd7157a/railties/lib/generator/named_base.rb#L56-57, and it appears that the plural name is always pluralized explicitly while the singular name is more or less directly taken from the name parameter. (I am guessing this might have something to do with the bug I mentioned earlier. @josevalim?) This seems wrong to me as it is clearly not how it is being used now (e.g. the resource generator passes in a plural name here when creating the controllers). Even if it's not wrong the naming makes it extremely confusing.
  3. I'm not sure where I should implement the check. (NamedBase is probably not a good place to put that logic as it will probably break a lot of generators out there.) Perhaps I should move this to railties/lib/rails/generators/rails/model/model_generator.rb? Would that work?

I really want this bug fixed (if this get fixed, it'll be my favourite new feature in Rails 4!), and if someone can clarify things for me / provide some suggestions, I'm willing to spend some time coming up with a solution.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jun 21, 2012

Member

I really want this bug fixed (if this get fixed, it'll be my favourite new feature in Rails 4!), and if someone can clarify things for me / provide some suggestions, I'm willing to spend some time coming up with a solution.

You will probably get better feedback from the rails core mailing list, honestly.

Member

steveklabnik commented Jun 21, 2012

I really want this bug fixed (if this get fixed, it'll be my favourite new feature in Rails 4!), and if someone can clarify things for me / provide some suggestions, I'm willing to spend some time coming up with a solution.

You will probably get better feedback from the rails core mailing list, honestly.

@andmej

This comment has been minimized.

Show comment
Hide comment
@andmej

andmej Jun 22, 2012

Contributor

Sounds like a great idea if you can get it working.

Contributor

andmej commented Jun 22, 2012

Sounds like a great idea if you can get it working.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jun 22, 2012

Member

I posted this to the core mailing list per @steveklabnik's suggestion - link here https://groups.google.com/forum/?fromgroups#!topic/rubyonrails-core/X9Ww8eLM_t4.

Member

chancancode commented Jun 22, 2012

I posted this to the core mailing list per @steveklabnik's suggestion - link here https://groups.google.com/forum/?fromgroups#!topic/rubyonrails-core/X9Ww8eLM_t4.

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 19, 2012

Member

👍 on this if pending reasonable implementation. You mentioned that we're already doing this check and fix in the scaffold generator, how are we doing it there?

In your post you also had some implementation questions, if you can get a working solution and do a pull request cc me and we can talk about implementation.

Member

schneems commented Jul 19, 2012

👍 on this if pending reasonable implementation. You mentioned that we're already doing this check and fix in the scaffold generator, how are we doing it there?

In your post you also had some implementation questions, if you can get a working solution and do a pull request cc me and we can talk about implementation.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Jul 19, 2012

Member

Thanks Richard for digging up this thread! :)

This is still in the back of my mind, and I'll get to it as soon as I have time. (Do you have an idea roughly how much time I have before Rails 4 hits feature freeze? Weeks/Month or two/Months?)

In the scaffold generator (actually, the ResourceHelpers) we simply check if the name is plural, if so we convert that to the singular form and emits the warning before passing the parameter down to the model generator, unless there is the --force-plural flag.

So we could either:

  1. Duplicate that check in the models generator
  2. Move the check down from the scaffold generator to the model generator

#1 is not so DRY, plus we'll have to communicate the --force-plural flag downstream as well as somehow avoid printing the error twice

#2: Would need to check if anything other than the models generator (views/helpers/tests etc) are expecting the name to be singular

Also, it's unclear what really is the "model generator", because the model generator actually gets most of its functionality from NamedBase, plus there is and then it passes the duty onto the ActiveModel -> ActiveRecord generators. So it'll take some work for me to figure out where exactly is the right place to do this without breaking other ORMs - unless someone more knowledgable could point me to the right places (which is why I started the original thread :)

Godfrey

Member

chancancode commented Jul 19, 2012

Thanks Richard for digging up this thread! :)

This is still in the back of my mind, and I'll get to it as soon as I have time. (Do you have an idea roughly how much time I have before Rails 4 hits feature freeze? Weeks/Month or two/Months?)

In the scaffold generator (actually, the ResourceHelpers) we simply check if the name is plural, if so we convert that to the singular form and emits the warning before passing the parameter down to the model generator, unless there is the --force-plural flag.

So we could either:

  1. Duplicate that check in the models generator
  2. Move the check down from the scaffold generator to the model generator

#1 is not so DRY, plus we'll have to communicate the --force-plural flag downstream as well as somehow avoid printing the error twice

#2: Would need to check if anything other than the models generator (views/helpers/tests etc) are expecting the name to be singular

Also, it's unclear what really is the "model generator", because the model generator actually gets most of its functionality from NamedBase, plus there is and then it passes the duty onto the ActiveModel -> ActiveRecord generators. So it'll take some work for me to figure out where exactly is the right place to do this without breaking other ORMs - unless someone more knowledgable could point me to the right places (which is why I started the original thread :)

Godfrey

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 17, 2012

Member

Since this is a feature request, I'm giving it a close. @chancancode , please submit a pull.

Member

steveklabnik commented Nov 17, 2012

Since this is a feature request, I'm giving it a close. @chancancode , please submit a pull.

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