Skip to content

Commit

Permalink
+ #find_by_id is the new default for populate_with
Browse files Browse the repository at this point in the history
  • Loading branch information
floere committed Nov 19, 2012
1 parent e3014a0 commit 2052f1a
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 6 deletions.
18 changes: 13 additions & 5 deletions client/lib/picky-client/convenience.rb
Expand Up @@ -36,7 +36,9 @@ def total

# Populates the ids with (rendered) model instances.
#
# Give it an AR class and options for the find and it
# It does this by calling #find_by_id on the given model class.
#
# Give it eg. an ActiveRecord class and options for the find_by_id and it
# will yield each found result for you to render.
#
# If you don't pass it a block, it will just use the AR results.
Expand All @@ -50,12 +52,14 @@ def total
# === Options
# * up_to: Amount of results to populate. All of them by default.
# * finder_method: Specify which AR finder method you want to load the model with. Default is #find_by_id.
# * The rest of the options are directly passed through to the ModelClass.find(ids, options) method. Default is {}.
# * The rest of the options are directly passed through to the ModelClass.find_by_id(ids, options) method. Default is {}.
#
def populate_with model_class, options = {}, &block
the_ids = ids options.delete(:up_to)
finder_method = options.delete(:finder_method) || :find_by_id

# Call finder method.
#
objects = model_class.send finder_method, the_ids, options

# Put together a mapping.
Expand All @@ -68,9 +72,13 @@ def populate_with model_class, options = {}, &block
# Preserves the order
#
objects = the_ids.map { |id| mapped_entries[id] }

objects.collect!(&block) if block


# Replace objects with rendered versions if a block is given.
#
objects.collect! &block if block

# Enhance the allocations with the objects or rendered objects.
#
amend_ids_with objects

objects
Expand Down
2 changes: 1 addition & 1 deletion client/spec/picky-client/convenience_spec.rb
Expand Up @@ -93,7 +93,7 @@ class ARClass
def initialize id
@id = id
end
def self.find ids, options = {}
def self.find_by_id ids, options = {}
ids.map { |id| new(id) }
end
def == other
Expand Down

8 comments on commit 2052f1a

@prami
Copy link
Contributor

@prami prami commented on 2052f1a Nov 25, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find_by_id in ActiveRecord returns only one object, not objects collection. Its a bug or feature? :)

@floere
Copy link
Owner Author

@floere floere commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the note. If you pass an Array as the first parameter, find_by_id behaves like find_all_by_id. Perhaps I should explicitly use find_all_by_id. Please correct me if I am wrong!

Update: This was wrong ;)

@prami
Copy link
Contributor

@prami prami commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass an Array as the first parameter find_by_id return object with id equal to the first element of the array.

@floere
Copy link
Owner Author

@floere floere commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which version are you talking about? My integration tests disagree with you.

Edit: No, my integration tests just don't test this ;) I'll look into it.

@prami
Copy link
Contributor

@prami prami commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talking about activerecord 3.2.8. Could you provide a link to this test?

@floere
Copy link
Owner Author

@floere floere commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Michał, you've found a bug :) A fix using find_all_by_id is released in 4.12.1. (Also see https://github.com/floere/picky/wiki/Contributions-and-contributors)

@prami
Copy link
Contributor

@prami prami commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@floere
Copy link
Owner Author

@floere floere commented on 2052f1a Nov 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a project you'd like presented, just add it to the contributors page :)

Please sign in to comment.