Removed deprecated options `:hint_class`, `:group_by`, `:group_label`, `:find_options` and `:value` #953

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
Owner

justinfrench commented Jul 11, 2013

Would love some review on this. Probably best to review commit-by-commit rather than a whole diff. Would particularly like review of 1fe253f, especially this line.

Collaborator

sobrinho commented Jul 11, 2013

Seems okay at first glance, I'm trusting on you travisci! 👍

Collaborator

twalpole commented Jul 11, 2013

Should the configure option default_hint_class now be changed to hint_class, since theres no way to override it anymore? Same with default_inline_error_class and default_error_list_class

Does this actually need the .all ? or can it just be written as reflection.klass.where(scope_conditions) for all rails versions? - maybe its necessary for 3.0 or 3.1, been a while since I've worked with them, but I dont think its necessary for 3.2

Owner

justinfrench replied Jul 11, 2013

Just checked the Rails 3.2 guides, you're right. If the Rails >= 3.2 branch goes in ahead of this (let's assume it does), this can be simpler. Thanks.

Collaborator

twalpole commented Jul 11, 2013

@justinfrench upon further review issue #954 affects the "remove deprecated :find_options option" and "refactor collection_from_association" changes. Summary -- rails4 got rid of the conditions option on associations and replaced it with a scope
has_many :somethings, conditions: { active: true }, ...
in rails 4 becomes
has_many :somethings, -> { where(active: true) }, ...
and the scope function is available on the reflection as scope. I think we should probably be handling this change.

Owner

justinfrench commented Jul 11, 2013

@twalpole great catch — how intertwined are these two issues? If the focus of this branch is removing old features (rather than adapting to Rails 4), can they be treated in isolation, or should we address #954 in here?

I'm also a little confused because in here you're using has_many in the example here (which we do need to handle) and Model.where in #954, which is still supported. I haven't had coffee yet though. Maybe it's a bad test that's confusing me because of the stubbing?

Owner

justinfrench commented Jul 11, 2013

@twalpole made a separate issue #955 for the configuration name — i suspect we have a few that don't make sense any more

Collaborator

twalpole commented Jul 11, 2013

@justinfrench I believe the has_many association in the model is reflected on to get the klass and then the :conditions option is applied to that class using where to get the collection - I may be confusing all of this, but thats my current understanding :). They can definitely be handled in isolation by removing the test for conditions options on assocations from rails 4 test runs, and then for rails 4 compatibility I think we need to handle the scope option now allowed on associations.

👍 for the work!

Owner

justinfrench commented Jul 26, 2014

Looks like this was achieved in another branch, closing.

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