Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1 #36029

Merged
merged 1 commit into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@kamipo
Copy link
Member

commented Apr 19, 2019

where.not with polymorphic association is partly fixed incidentally at
213796f (refer #33493, #26207, #17010, #16983, #14161), and I've added
test case e9ba12f to avoid lose that fix accidentally in the future.

In Rails 5.2, where.not(polymorphic: object) works as expected as
NAND, but where.not(polymorphic_type: object.class.polymorphic_name, polymorphic_id: object.id) still unexpectedly works as NOR.

To will make where.not working desiredly as NAND in Rails 6.1, this
deprecates where.not working as NOR. If people want to continue NOR
conditions, we'd encourage to them to where.not each conditions
manually.

all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)]
assert_equal all, PriceEstimate.all.map(&:estimate_of)

In Rails 6.0:

sapphire = treasures(:sapphire)

nor = all.reject { |e|
  e.estimate_of_type == sapphire.class.polymorphic_name
}.reject { |e|
  e.estimate_of_id == sapphire.id
}
assert_equal [cars(:honda)], nor

without_sapphire = PriceEstimate.where.not(
  estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
)
assert_equal nor, without_sapphire.map(&:estimate_of)

In Rails 6.1:

sapphire = treasures(:sapphire)

nand = all - [sapphire]
assert_equal [treasures(:diamond), cars(:honda)], nand

without_sapphire = PriceEstimate.where.not(
  estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
)
assert_equal nand, without_sapphire.map(&:estimate_of)

Resolves #31209.

@rails-bot rails-bot bot added the activerecord label Apr 19, 2019

Deprecate `where.not` working as NOR and will be changed to NAND in R…
…ails 6.1

`where.not` with polymorphic association is partly fixed incidentally at
213796f (refer #33493, #26207, #17010, #16983, #14161), and I've added
test case e9ba12f to avoid lose that fix accidentally in the future.

In Rails 5.2, `where.not(polymorphic: object)` works as expected as
NAND, but `where.not(polymorphic_type: object.class.polymorphic_name,
polymorphic_id: object.id)` still unexpectedly works as NOR.

To will make `where.not` working desiredly as NAND in Rails 6.1, this
deprecates `where.not` working as NOR. If people want to continue NOR
conditions, we'd encourage to them to `where.not` each conditions
manually.

```ruby
all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)]
assert_equal all, PriceEstimate.all.map(&:estimate_of)
```

In Rails 6.0:

```ruby
sapphire = treasures(:sapphire)

nor = all.reject { |e|
  e.estimate_of_type == sapphire.class.polymorphic_name
}.reject { |e|
  e.estimate_of_id == sapphire.id
}
assert_equal [cars(:honda)], nor

without_sapphire = PriceEstimate.where.not(
  estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
)
assert_equal nor, without_sapphire.map(&:estimate_of)
```

In Rails 6.1:

```ruby
sapphire = treasures(:sapphire)

nand = all - [sapphire]
assert_equal [treasures(:diamond), cars(:honda)], nand

without_sapphire = PriceEstimate.where.not(
  estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
)
assert_equal nand, without_sapphire.map(&:estimate_of)
```

Resolves #31209.

@kamipo kamipo force-pushed the kamipo:deprecate_where_not branch from 95569d0 to 12a9664 Apr 19, 2019

@kamipo

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

I'll going to merge the deprecation in 6.0 to will fix where.not in 6.1.
Let me know if anyone has any objection.

@kamipo kamipo merged commit 9d1f9b9 into rails:master Apr 23, 2019

2 checks passed

buildkite/rails Build #60590 passed (9 minutes, 10 seconds)
Details
codeclimate All good!
Details

@kamipo kamipo deleted the kamipo:deprecate_where_not branch Apr 23, 2019

if not_behaves_as_nor?(opts)
ActiveSupport::Deprecation.warn(<<~MSG.squish)
NOT conditions will no longer behave as NOR in Rails 6.1.
To continue using NOR conditions, NOT each conditions manually

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 23, 2019

Member

This message is confusing. How about?

To continue using NOR conditions, chain each `NOT` condition manually.

We probably should add a new method that do the NAND too. Maybe invert as suggested by @matthewd?

This comment has been minimized.

Copy link
@kamipo

kamipo Apr 23, 2019

Author Member

I'm not sure that people want to hurry to migrate to new method, since we have left this issue alone for 5 years (oldest one #14161 I can find).

This issue has already mitigated by 213796f (e9ba12f), and will eventually be fixed even if new method isn't introduced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.