Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #9346 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[PATCH] Don't clobber :extend options with a block

Reported by: josh Assigned to: nzkoz
Priority: normal Milestone: 1.x
Component: ActiveRecord Version: edge
Severity: normal Keywords: associations verified
Cc: tarmo, mislav

Description

Right now, blocks passed to an association macro clobber the :extend options. This patch merges the :extend options and block instead. 

has_many :projects, :extend => SomeAssociationExtension do
  def find_least_recent
    find(:first, :order => "id ASC")
  end
end

Attachments

associations_extend_modules_and_block.diff (3.5 kB) - added by josh on 09/01/07 20:03:13.

Change History

08/23/07 00:07:41 changed by tarmo

  • cc set to tarmo.

Makes sense, looks good, tests pass.

+1

(follow-up: ↓ 3 ) 08/23/07 07:46:09 changed by mislav

  • cc changed from tarmo to tarmo, mislav.

+1

Everything is great but the naming. I don't like this method's signature: 

def create_extension_module(association_id, extension, other_extensions)

The method does more than its name suggests, also. Why not changing it to something like this: 

def extension_modules(association_id, block_extension, extensions)

The method name is now plural and the arguments are more descriptive.

And why do you do this? 

other_extensions = [other_extensions].flatten.compact

A user should be required to enter a flattened array - no need for this. Simply: 

(other_extensions || []).push(extension_module_name.constantize)

(in reply to: ↑ 2 ) 08/23/07 19:03:34 changed by josh

Replying to mislav:

The method does more than its name suggests, also. Why not changing it to something like this:

Yep.

Replying to mislav:

A user should be required to enter a flattened array - no need for this.

:extend can be pass a Module or an array of modules. You need move the module into an Array because a single Module doesn't support push.

I wanted to use the .to_a but its deprecated for some reason. 

extensions.to_a.push(extension_module_name.constantize)

# but

irb(main):003:0> Module.to_a
(irb):3: warning: default `to_a' will be obsolete

08/23/07 19:30:04 changed by tarmo

You can use Array[*extensions] instead of extensions.to_a.

08/23/07 19:36:13 changed by mislav

Ohh, I'm dumb. Thanks for correcting me.

Tarmo makes a good point.

08/23/07 19:43:11 changed by josh

Thanks Tarmo. For the Array[*] tip.

08/23/07 19:50:40 changed by josh

Darn, we still have to use compact. 

extensions = nil
Array[*extensions] == [nil]
extensions.to_a == []

Why must it be obsolete! :)

08/28/07 05:18:18 changed by nzkoz

You want 

Array(extensions)

I've already got some daft code in rails where this would've sufficed, so I'm speaking from experience :)

08/28/07 05:24:19 changed by nzkoz

  • owner changed from david to nzkoz.
  • status changed from new to assigned.

09/01/07 20:03:13 changed by josh

  • attachment associations_extend_modules_and_block.diff added.

09/01/07 20:03:40 changed by josh

Updated.

Thx Koz

09/16/07 17:28:29 changed by josh

  • keywords changed from associations to associations verified.

I'll assume a +1 from Koz since he didn't mention anything bad ;)

09/17/07 21:19:48 changed by bitsweat

  • status changed from assigned to closed.
  • resolution set to fixed.

(In [7504]) Associations macros accept extension blocks alongside modules. Closes #9346.

10/27/07 20:31:13 changed by bitsweat

(In [8046]) Allow association redefinition in subclasses. Closes #9346.

10/27/07 20:32:38 changed by bitsweat

Oops wrong ticket.

10/27/07 21:15:20 changed by bitsweat

(In [8048]) Merge [8046] from trunk: allow association redefinition in subclasses. References #9346.