Skip to content
This repository was archived by the owner on Jun 30, 2018. It is now read-only.

Change #find to #where to prevent ActiveRecord::RecordNotFound#702

Closed
jeyb wants to merge 1 commit into
karmi:masterfrom
jeyb:find_to_where
Closed

Change #find to #where to prevent ActiveRecord::RecordNotFound#702
jeyb wants to merge 1 commit into
karmi:masterfrom
jeyb:find_to_where

Conversation

@jeyb

@jeyb jeyb commented Apr 11, 2013

Copy link
Copy Markdown

As per @karmi's comments on #673 I've only used where when the class inherits ActiveRecord::Base.

Thoughts @karmi?

As per @karmi's comments on #673 I've
only used where when the class inherits ActiveRecord::Base.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're using Ruby 1.9 Hash syntax here. I don't know the general policy here (I just added my first pull request myself) but the other tests (at least the unit tests) use the 1.8 Hash syntax. But from some older commits and the documentation you can see that 1.8 compatibility is desired.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yes. Ruby 1.8 compatibility of the whole gem is in fact required, the only place where we commonly use Ruby 1.9 syntax is integration tests (easier on the eyes, most people are on Ruby 1.9, Hash preserves ordering, etc). The unit tests have to use the Ruby 1.8 syntax, because we run the unit tests suite on Ruby 1.8 on Travis (and of course locally).

@karmi

karmi commented Apr 12, 2013

Copy link
Copy Markdown
Owner

@jeyb See also discussion in #602, please.

@jeyb

jeyb commented Apr 12, 2013

Copy link
Copy Markdown
Author

@karmi @robinbrandt I can definitely update to Ruby 1.8 syntax.

@karmi If you can point out where I can find a list of supported ORMs I can work on a Strategy implementation like done here efb9c14. Is this already being worked on? Don't want to dup effort. But this fix is critical for my project so I do wan't to work on it soon.

@brupm

brupm commented May 9, 2013

Copy link
Copy Markdown
Contributor

@karmi bump

@karmi

karmi commented May 11, 2013

Copy link
Copy Markdown
Owner

Sorry it hangs in there like this for so long. Any news on this in the meantime? I think you should synchronize with @timoschilling to work out some elegant solution which would pick correct strategy for __find_records_by_ids for ActiveRecord, Mongoid, etc. Let's keep the current implementation as default.

@karmi

karmi commented Jul 21, 2013

Copy link
Copy Markdown
Owner

Just wondering about the state of this ticket -- I'm all for exposing ORM-specific finder methods in the __find_records_by_ids, but would like to have it done properly, so we can extend it, covered it by tests, etc. Currently, for a homogenous application, people can just override the method, but it would be cool to have a proper out-of-the-box support here...

@jeyb

jeyb commented Jul 26, 2013

Copy link
Copy Markdown
Author

Didn't get a chance to work on this till a few minutes ago. How does this look? #810

@karmi

karmi commented Oct 2, 2013

Copy link
Copy Markdown
Owner

Closing in favour of #810.

@karmi karmi closed this Oct 2, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants