rails-style-guide
rails-style-guide copied to clipboard
rubocop
Add section for `where.not` with multiple attributes
I find the original reasoning confusing:
Expected behavior NOT(A AND B) = NOT(A) OR NOT(B)
To me, both where.not(a: 1, b: 2) and where.not(a: 1).where.not(b: 2) should be NOT(A) AND NOT(B).
I agree with the bad example:
# bad
User.where.not(status: "active", plan: "basic")
But I don't agree with the message involving boolean algebra reasoning. I'd say:
Avoid passing multiple attributes to a
where.not, as Rails logic in this case has changed in Rails 6.1 and will now yield results matching either of those conditions, e.g.where.not(status: 'active', plan: 'basic')would return records withactivestatuswhen theplanisbusiness.
@andyw8 WDYT?
Personally I would avoid not except for trivial cases with a single attribute, but I also wouldn't use a chained not as it can still be highly confusing. Instead I would write it as SQL to be explicit about the intent.
I side with @andyw8 and would recommend not using where.not with multiple attributes or chained.
The few real-world usages of where.not with multiple attributes I could find in real-world-rspec repos:
open-source-billing/app/helpers/application_helper.rb
415: PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').where(is_read: false).count
423: PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').order('created_at desc').page(1).per(10) if current_user.present?
open-source-billing/app/controllers/notifications_controller.rb
3: @activities = PublicActivity::Activity.where.not(owner_id: current_user.id, key: 'client.update').order('created_at desc').page(params[:page].to_i + 1).per(10) if current_use
r.present?
gitlabhq/app/models/concerns/milestoneable.rb
22: scope :without_particular_release, -> (tag, project_id) { joins_milestone_releases.where.not(milestones: { releases: { tag: tag, project_id: project_id } }) }
I have no confidence regarding chained where.not(...).where.not(...) calls, but I imagine there should be even less of them in practice.
WDYT of discouraging using it? @tejasbubane Would you adjust the PR?
