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

Add delete_by/destroy_by as relation methods #35304

Closed
dhh opened this Issue Feb 17, 2019 · 8 comments

Comments

Projects
None yet
4 participants
@dhh
Copy link
Member

dhh commented Feb 17, 2019

We have create_by and find_by to create and find records by the passed parameters, but we don't have the same for delete or destroying. That's a missing symmetry.

# Before
unreads.find_by(readable: readable)&.destroy
unreads.where(readable: readable).destroy_all
unreads.where(readable: readable).delete_all

# After
unreads.destroy_by(readable: readable)
unreads.destroy_by(readable: readable)
unreads.delete_by(readable: readable)

@dhh dhh added the activerecord label Feb 17, 2019

@dhh dhh added this to the 6.0.0 milestone Feb 17, 2019

@ahorek

This comment has been minimized.

Copy link

ahorek commented Feb 17, 2019

so destroy_by should destroy all records or a single record by the condition?
I don't see a use case for deleting a single record that isn't already uniq like id. Could you explain your use case in more details?
If it should delete all records, than it isn't symetric with find by.
Post.where(readable: true).destroy_all
or
Post.destroy_by(readable: true)
these two should be equal?

@dhh

This comment has been minimized.

Copy link
Member Author

dhh commented Feb 17, 2019

#destroy_by should destroy everything that matches the conditions. So yes, essentially it's a destroy_all, but that's redundant to state since there's a condition within. The symmetry is in spirit, not in operation. That you can find/create/destroy/delete with a single command that combines the scope and the operation.

@dhh

This comment has been minimized.

Copy link
Member Author

dhh commented Feb 17, 2019

Here's the basic implementation I'm just using in my app at the moment:

module ActiveRecordDestroyAndDeleteBy
  def destroy_by(conditions)
    where(conditions).destroy_all
  end
  
  def delete_by(conditions)
    where(conditions).delete_all
  end
end

ActiveRecord::Relation.include ActiveRecordDestroyAndDeleteBy
@ahorek

This comment has been minimized.

Copy link

ahorek commented Feb 17, 2019

Ok, thanks for the clarification, but I would find confusing that #find_by works with a single record and a method with a simmilar name behaves quite differently.

@dhh

This comment has been minimized.

Copy link
Member Author

dhh commented Feb 18, 2019

@abhaynikam

This comment has been minimized.

Copy link
Contributor

abhaynikam commented Feb 18, 2019

@dhh : can I take this up?

@javan

This comment has been minimized.

Copy link
Member

javan commented Feb 18, 2019

I would find confusing that #find_by works with a single record and a method with a similar name behaves quite differently

+1

Perhaps

unreads.destroy_by(readable: readable) # destroy one
unreads.destroy_where(readable: readable) # destroy many
unreads.delete_where(readable: readable) # delete many

?

@dhh

This comment has been minimized.

Copy link
Member Author

dhh commented Feb 18, 2019

I don't think the distinction is meaningful. It's meaningful when it comes to find_all_by/find_by primarily because it governs the return value, whether to return an array or a record. That doesn't matter here. If the conditions match 1 record, it'll delete/destroy that record. If it matches many, it'll delete/destroy many.

I just went through and replace all the usage in my current app, and the singular v plural distinction does not offer any confusion in any of those cases.

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.