Extract the short-hand column methods into ColumnMethods
#19030
Conversation
| :uuid, | ||
| :xml, | ||
| ].each do |column_type| | ||
| define_method column_type do |*args, **options| |
There was a problem hiding this comment.
Methods defined using define_method are slightly slower to execute than those defined by def so not recommended for hotspots. Similarly, splatting and globbing *args for most methods that only have one (all but xml and tsvector here) could be a performance penalty.
Perhaps make a define_column_type factory method that goes something like define_column_for('something', args: boolean) (false by default) which would generate the appropriate method using the pattern:
eval <<-EOS
def(...)
...
end
end
Then you can build up on that further with aggregate method like:
define_column_types_for(:bigserial, :bigint, ...)
define_column_types_for(:xml, :tsvector, args: true)
kamipo
force-pushed
the
extract_short_hand_column_methods
branch
from
4ef2630
to
621f17a
Compare
|
@egilburg Thank you for your review. I think the migration is not a hotspot of the performance, but I agree to prefer |
| with_change_table do |t| | ||
| @connection.expect :add_column, nil, [:delete_me, :foo, :xml, {}] | ||
| @connection.expect :add_column, nil, [:delete_me, :bar, :xml, {}] | ||
| t.xml :foo, :bar |
There was a problem hiding this comment.
I think it is a bug that the short-hand methods can't define multiple columns.
kamipo
force-pushed
the
extract_short_hand_column_methods
branch
from
621f17a
to
24eb440
Compare
|
@sgrif Thank you for your review. I have changed that do not meta-programming for the API site. |
Extract the short-hand column methods into `ColumnMethods`
Only `primary_key` should be extracted by d47357e in rails#19030, but `new_coclumn_definition` was also extracted because rails#17631 is merged previously, then rails#19030 is auto merged without conflicts. This commit is for move back `new_column_definition` into `TableDefinition`.
The short-hand methods was added by rails#19030. schema dumping support was added by rails#19185. This commit is follow up to these PRs.
Definition of the short-hand column methods is duplicated in
TableandTableDefinition.However,
biginthave not been added toTable. Such problem can be prevented byextracting the methods into
ColumnMethods.In addition, the short-hand column methods should be able to define multiple columns,
but
PostgreSQL::ColumnMethodswas not able to.