When generating a model/migration from cli added ability to specify whether particular property should also have an index #2555

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
7 participants
@german
Contributor

german commented Aug 17, 2011

When generating a model/migration from cli wouldn't that be nice if there would be an ability to specify whether particular property should also have an index like this:

rails g model person name:string:index profile:string

so developer could just run 'rake db:migrate' w/o looking into migration file?

The main change is adding the has_index property in the railties/lib/rails/generators/generated_attribute.rb:

module Rails
  module Generators
    class GeneratedAttribute
      attr_accessor :name, :type, :has_index

      def initialize(name, type, has_index = false)
        type = :string if type.blank?
        @name, @type, @has_index = name, type.to_sym, has_index.eql?("index")
      end

      ...

      def has_index?
        @has_index
      end
    end
  end
end

What do you think of it? Was it worth implementing?

UPD: added ability to specify :limit and :precision/:scale column options in migration like this: name:string[40] or name:string[40]:index or price:decimal[5.2] as proposed below (I chosen square brackets because parentheses break bash shell)

UPD changed a dot to a comma and square brackets to curly ones while specifying column type options

Dmitrii Samoilov
added ability to specify from cli when generating a model/migration w…
…hether particular property should be an index like this 'rails g model person name:string:index profile:string'
@@ -154,8 +154,8 @@ module Rails
# Convert attributes array into GeneratedAttribute objects.
def parse_attributes! #:nodoc:
self.attributes = (attributes || []).map do |key_value|
- name, type = key_value.split(':')
- Rails::Generators::GeneratedAttribute.new(name, type)
+ name, type, has_index = key_value.split(':')

This comment has been minimized.

@vijaydev

vijaydev Aug 17, 2011

Member

Rails 3.2 (edge) has a change where the type can be omitted and it will default to string. So you need to support "name:index" in addition to "name:string:index".

@vijaydev

vijaydev Aug 17, 2011

Member

Rails 3.2 (edge) has a change where the type can be omitted and it will default to string. So you need to support "name:index" in addition to "name:string:index".

This comment has been minimized.

@vijaydev

vijaydev Aug 17, 2011

Member

Here is the relevant commit: 08983fe

@vijaydev

vijaydev Aug 17, 2011

Member

Here is the relevant commit: 08983fe

This comment has been minimized.

@german

german Aug 17, 2011

Contributor

yes, I see

@german

german Aug 17, 2011

Contributor

yes, I see

This comment has been minimized.

@german

german Aug 17, 2011

Contributor

This comment has been minimized.

@josevalim

josevalim Dec 23, 2011

Contributor

What if we remove this code from here and move it to Rails::Generators::GeneratedAttribute.parse(key_value). That said, the responsibility ends up all in one place.

@josevalim

josevalim Dec 23, 2011

Contributor

What if we remove this code from here and move it to Rails::Generators::GeneratedAttribute.parse(key_value). That said, the responsibility ends up all in one place.

This comment has been minimized.

@german

german Dec 23, 2011

Contributor

You mean move this code to Rails::Generators::GeneratedAttribute#parse(key_value):

   name, type, has_index = key_value.split(':')
   # if user provided "name:index" instead of "name:string:index" type should be set blank
   # so GeneratedAttribute's constructor could set it to :string
   if type.eql?("index")
     has_index = type
     type = nil
   end

So we could write Rails::Generators::GeneratedAttribute.parse(key_value) ? Sure, I think it's ok.

@german

german Dec 23, 2011

Contributor

You mean move this code to Rails::Generators::GeneratedAttribute#parse(key_value):

   name, type, has_index = key_value.split(':')
   # if user provided "name:index" instead of "name:string:index" type should be set blank
   # so GeneratedAttribute's constructor could set it to :string
   if type.eql?("index")
     has_index = type
     type = nil
   end

So we could write Rails::Generators::GeneratedAttribute.parse(key_value) ? Sure, I think it's ok.

This comment has been minimized.

@josevalim

josevalim Dec 23, 2011

Contributor

Yes, exactly.

@josevalim

josevalim Dec 23, 2011

Contributor

Yes, exactly.

Dmitrii Samoilov
added condition to check whether user omitted type of the attribute b…
…ut specified it should have index while generating model/migration from the cli, so 'rails g model person name:index age:integer' should work
@alexeymuranov

This comment has been minimized.

Show comment
Hide comment
@alexeymuranov

alexeymuranov Aug 31, 2011

Contributor

I like the idea. It would also be nice to be able to specify the length as name:string(40).

Contributor

alexeymuranov commented Aug 31, 2011

I like the idea. It would also be nice to be able to specify the length as name:string(40).

@german

This comment has been minimized.

Show comment
Hide comment
@german

german Sep 1, 2011

Contributor

@alexeymuranov yeah, I didn't thought of that. Will try to implement it.

Contributor

german commented Sep 1, 2011

@alexeymuranov yeah, I didn't thought of that. Will try to implement it.

@german

This comment has been minimized.

Show comment
Hide comment
@german

german Sep 2, 2011

Contributor

@alexeymuranov I think that's what you've talked about: name:string[40] or name:string[40]:index or price:decimal[5.2]. I choose square brackets because parentheses break bash shell

Contributor

german commented Sep 2, 2011

@alexeymuranov I think that's what you've talked about: name:string[40] or name:string[40]:index or price:decimal[5.2]. I choose square brackets because parentheses break bash shell

@alexeymuranov

This comment has been minimized.

Show comment
Hide comment
@alexeymuranov

alexeymuranov Sep 2, 2011

Contributor

Thanks!

Contributor

alexeymuranov commented Sep 2, 2011

Thanks!

@sikachu

This comment has been minimized.

Show comment
Hide comment
@sikachu

sikachu Sep 2, 2011

Member

Square brackets need to be escaped in zsh as well ...

Member

sikachu commented Sep 2, 2011

Square brackets need to be escaped in zsh as well ...

@german

This comment has been minimized.

Show comment
Hide comment
@german

german Sep 5, 2011

Contributor

Thanks for the feedback @sikachu. So what do you think of which delimiter will do ideally? Comma maybe (like "name:string,20") or curly brackets (like "name:string{20}:index") or dot? Or should I delete the last commit at all?

Contributor

german commented Sep 5, 2011

Thanks for the feedback @sikachu. So what do you think of which delimiter will do ideally? Comma maybe (like "name:string,20") or curly brackets (like "name:string{20}:index") or dot? Or should I delete the last commit at all?

@alexeymuranov

This comment has been minimized.

Show comment
Hide comment
@alexeymuranov

alexeymuranov Sep 5, 2011

Contributor

Sorry for starting this :). Maybe underscore? name:string_20:index
Or just name:string20:index

Contributor

alexeymuranov commented Sep 5, 2011

Sorry for starting this :). Maybe underscore? name:string_20:index
Or just name:string20:index

@dmathieu

This comment has been minimized.

Show comment
Hide comment
@dmathieu

dmathieu Sep 5, 2011

Contributor

Is this really useful ? I don't think a lot of people will use it, it's too hard to remember.
And when you want to create a really specific migration with specific indexes, you can still manually edit the file.

Contributor

dmathieu commented Sep 5, 2011

Is this really useful ? I don't think a lot of people will use it, it's too hard to remember.
And when you want to create a really specific migration with specific indexes, you can still manually edit the file.

@alexeymuranov

This comment has been minimized.

Show comment
Hide comment
@alexeymuranov

alexeymuranov Sep 5, 2011

Contributor

About the parentheses: i thought that the parentheses syntax was natural, and this is how annotate_models writes its comments, but if parentheses do not work in bash, i do not know.
In my opinion, there is some use in all this: it is easier to edit several command lines before pasting them into the shell, than edit multiple migration files afterwards.

Contributor

alexeymuranov commented Sep 5, 2011

About the parentheses: i thought that the parentheses syntax was natural, and this is how annotate_models writes its comments, but if parentheses do not work in bash, i do not know.
In my opinion, there is some use in all this: it is easier to edit several command lines before pasting them into the shell, than edit multiple migration files afterwards.

@german

This comment has been minimized.

Show comment
Hide comment
@german

german Sep 5, 2011

Contributor

@dmathieu well, I don't know about :limit/:precision/:scale options but at least :index option might be useful, I often add indices by myself after generating a migration and I thought it could be automated somehow.

Contributor

german commented Sep 5, 2011

@dmathieu well, I don't know about :limit/:precision/:scale options but at least :index option might be useful, I often add indices by myself after generating a migration and I thought it could be automated somehow.

@Bodacious

This comment has been minimized.

Show comment
Hide comment
@Bodacious

Bodacious Sep 5, 2011

Contributor

Just noticed this pull request
+1 - great idea

Contributor

Bodacious commented Sep 5, 2011

Just noticed this pull request
+1 - great idea

@alexeymuranov

This comment has been minimized.

Show comment
Hide comment
@alexeymuranov

alexeymuranov Oct 13, 2011

Contributor

@german, just a thought: use parenthesis (``) for :limit, and those who need them from the command line, like me, will escape them!

Contributor

alexeymuranov commented Oct 13, 2011

@german, just a thought: use parenthesis (``) for :limit, and those who need them from the command line, like me, will escape them!

@german

This comment has been minimized.

Show comment
Hide comment
@german

german Oct 14, 2011

Contributor

@alexeymuranov I think I'll better create a thread in rails-core google group, so this pull request could attract more attention and gather some more opinions
/ cc @spastorino

Contributor

german commented Oct 14, 2011

@alexeymuranov I think I'll better create a thread in rails-core google group, so this pull request could attract more attention and gather some more opinions
/ cc @spastorino

@german german closed this Nov 24, 2011

@german german reopened this Dec 15, 2011

type = :string if type.blank?
- @name, @type = name, type.to_sym
+ @name, @type, @attr_options, @has_index = name, *parse_type_and_options(type), has_index.eql?("index")

This comment has been minimized.

@josevalim

josevalim Dec 23, 2011

Contributor

We don't need to do everything in one line. :)

@josevalim

josevalim Dec 23, 2011

Contributor

We don't need to do everything in one line. :)

This comment has been minimized.

@german

german Dec 23, 2011

Contributor

yes, I know)

@german

german Dec 23, 2011

Contributor

yes, I know)

@@ -3,7 +3,7 @@
module ActiveRecord
module Generators
class MigrationGenerator < Base
- argument :attributes, :type => :array, :default => [], :banner => "field:type field:type"
+ argument :attributes, :type => :array, :default => [], :banner => "field:type field:type field:type:index"

This comment has been minimized.

@josevalim

josevalim Dec 23, 2011

Contributor

Let's change the banner to: "field field:type field:type:index". What do you think?

@josevalim

josevalim Dec 23, 2011

Contributor

Let's change the banner to: "field field:type field:type:index". What do you think?

This comment has been minimized.

@german

german Dec 23, 2011

Contributor

Ok, cool. Actually I'm in doubt whether 'name:string[40]' functionality is needed as it was proposed ealier #2555 (comment) ? what do you think?

@german

german Dec 23, 2011

Contributor

Ok, cool. Actually I'm in doubt whether 'name:string[40]' functionality is needed as it was proposed ealier #2555 (comment) ? what do you think?

This comment has been minimized.

@josevalim

josevalim Dec 24, 2011

Contributor

I think it would be too hard to put all this information in the banner. We should document it in the USAGE though. Now that we use {} for extra information, we could even change the banner to: field[:type][:index] field[:type][:index] and put the remaining information in the USAGE.

@josevalim

josevalim Dec 24, 2011

Contributor

I think it would be too hard to put all this information in the banner. We should document it in the USAGE though. Now that we use {} for extra information, we could even change the banner to: field[:type][:index] field[:type][:index] and put the remaining information in the USAGE.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 23, 2011

Contributor

I like [] because this is what rake uses.

Contributor

josevalim commented Dec 23, 2011

I like [] because this is what rake uses.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 23, 2011

Contributor

Btw, decimal[5.2] meaning precision.scale is not good. I would suggest using decimal[5,2] or removing it.

Contributor

josevalim commented Dec 23, 2011

Btw, decimal[5.2] meaning precision.scale is not good. I would suggest using decimal[5,2] or removing it.

+
+ # parse possible attribute options like :limit for string/text/binary/integer or :precision/:scale for decimals
+ # when declaring options square brackets should be used since bash interpreter fails when parentheses are used
+ def parse_type_and_options(type)

This comment has been minimized.

@josevalim

josevalim Dec 23, 2011

Contributor

As I said, this could be the class method parse and the object would be then initialized with all four args (name, type, index and options).

@josevalim

josevalim Dec 23, 2011

Contributor

As I said, this could be the class method parse and the object would be then initialized with all four args (name, type, index and options).

Dmitrii Samoilov
changed square brackets to curly ones in column type definition, also…
… added ability to specify (uniq|unique) in index definition, so now it's possible to write 'discount:decimal{5,2}:uniq' or 'user_uuid:uniq' while generating new model/migration
@german

This comment has been minimized.

Show comment
Hide comment
@german

german Dec 23, 2011

Contributor

I've

  • changed square brackets to the curly ones in column type definition (so zsh and bash are ok)
  • changed a dot to a comma
  • added ability to specify (uniq|unique) in column definition (instead of index)

so now it's possible to write 'discount:decimal{5,2}:unique' or 'user_uuid:uniq' while generating new model/migration.

What do you think of this? I'll investigate on this further tomorrow. Thanks.

Contributor

german commented Dec 23, 2011

I've

  • changed square brackets to the curly ones in column type definition (so zsh and bash are ok)
  • changed a dot to a comma
  • added ability to specify (uniq|unique) in column definition (instead of index)

so now it's possible to write 'discount:decimal{5,2}:unique' or 'user_uuid:uniq' while generating new model/migration.

What do you think of this? I'll investigate on this further tomorrow. Thanks.

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Dec 24, 2011

Contributor

I have merged this to a local branch. I will push it to master soon.

Contributor

josevalim commented Dec 24, 2011

I have merged this to a local branch. I will push it to master soon.

@josevalim josevalim closed this in 0152fe9 Dec 24, 2011

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