Deprecate `Class` handler in `PredicateBuilder` #17916

Merged
merged 1 commit into from Dec 4, 2014

Conversation

Projects
None yet
4 participants
@mrgilman
Contributor

mrgilman commented Dec 4, 2014

Users should pass strings to queries instead of classes

@sgrif sgrif added the activerecord label Dec 4, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 4, 2014

Member

Could you add a CHANGELOG entry?

Member

rafaelfranca commented Dec 4, 2014

Could you add a CHANGELOG entry?

@mrgilman

This comment has been minimized.

Show comment
Hide comment
@mrgilman

mrgilman Dec 4, 2014

Contributor

Sure thing.

Contributor

mrgilman commented Dec 4, 2014

Sure thing.

Deprecate `Class` handler in `PredicateBuilder`
Users should pass strings to queries instead of classes
@mrgilman

This comment has been minimized.

Show comment
Hide comment
@mrgilman

mrgilman Dec 4, 2014

Contributor

Added a CHANGELOG entry.

Contributor

mrgilman commented Dec 4, 2014

Added a CHANGELOG entry.

sgrif added a commit that referenced this pull request Dec 4, 2014

Merge pull request #17916 from mrgilman/deprecate-class-handler
Deprecate `Class` handler in `PredicateBuilder`

@sgrif sgrif merged commit 6c9ed6d into rails:master Dec 4, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@joergschray

This comment has been minimized.

Show comment
Hide comment
@joergschray

joergschray Dec 8, 2014

Contributor

Passing a class for the type column of a polymorphic relationship or an STI table makes perfect sense to me. Of course you can work around it by defining a visitor in Arel, but that's not readily accessible for most users. So I think the use case in the test should be fixed, but not the concept as such.

Contributor

joergschray commented Dec 8, 2014

Passing a class for the type column of a polymorphic relationship or an STI table makes perfect sense to me. Of course you can work around it by defining a visitor in Arel, but that's not readily accessible for most users. So I think the use case in the test should be fixed, but not the concept as such.

@sgrif

This comment has been minimized.

Show comment
Hide comment
@sgrif

sgrif Dec 8, 2014

Member

Why is passing Foo so much better than calling "Foo"? You can also call .name on the class if you need to take one as an argument. It simply adds cruft to our code for a type that has no semantics on its own.

Member

sgrif commented Dec 8, 2014

Why is passing Foo so much better than calling "Foo"? You can also call .name on the class if you need to take one as an argument. It simply adds cruft to our code for a type that has no semantics on its own.

@joergschray

This comment has been minimized.

Show comment
Hide comment
@joergschray

joergschray Dec 8, 2014

Contributor

For a namespaced class Foo is not equivalent to "Foo". Yes I can also call .name - but why is passing a symbol or an ActiveRecord instance allowed - I could also call .to_s or .id on it. What is the criteria to approve of one and disapprove of the other?

Contributor

joergschray commented Dec 8, 2014

For a namespaced class Foo is not equivalent to "Foo". Yes I can also call .name - but why is passing a symbol or an ActiveRecord instance allowed - I could also call .to_s or .id on it. What is the criteria to approve of one and disapprove of the other?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Dec 8, 2014

Member

What is the criteria to approve of one and disapprove of the other?

I'd say usage. Symbol and Active Record instance are more used.

Member

rafaelfranca commented Dec 8, 2014

What is the criteria to approve of one and disapprove of the other?

I'd say usage. Symbol and Active Record instance are more used.

pikender added a commit to vinsol-spree-contrib/spree_marketing that referenced this pull request Feb 3, 2017

Updated for Spree 3.2.0.rc2 and Rails 5
Fixed Deprecation Warning :: Passing a class as a value in an Active Record query is deprecated and will be removed. Pass a string instead.
Refer rails/rails#17916
Used Rspec stub_const to silence CONSTANT redefinition warnings
Pending: Failing Specs for acts_as_multilist concern
TRACE_START
Spree::Marketing::List::MostZoneWiseOrders behaves like acts_as_multilist .generator if list doesn't exists should change result by 1
      Failure/Error: it { expect { list_type.send :generator }.to change { list_type.all.count }.by 1 }
        expected result to have changed by 1, but was changed by 0
      Shared Example Group: "acts_as_multilist" called from ./spec/models/spree/marketing/list/most_zone_wise_orders_list_spec.rb:12
      # ./spec/shared/acts_as_multilist.rb:59:in `block (4 levels) in <top (required)>'
TRACE_END

@pikender pikender referenced this pull request in vinsol-spree-contrib/spree_marketing Feb 3, 2017

Open

[WIP] Updated for Spree 3.2.0.rc2 and Rails 5 #82

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