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

Introduce delete_by and destroy_by methods to ActiveRecord::Relation #35316

Merged
merged 1 commit into from Feb 19, 2019

Conversation

@abhaynikam
Copy link
Contributor

@abhaynikam abhaynikam commented Feb 18, 2019

closes #35304

Adds delete_by and destroy_by as relation methods to add missing symmetry.

@r? @dhh

@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch from 5d6b79b to a1413c7 Feb 18, 2019
@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch from a1413c7 to fdf10ba Feb 18, 2019
@dhh
Copy link
Member

@dhh dhh commented Feb 18, 2019

I think we can pare back the testing a lot. This is basically just a delegator to where + destroy/delete_all. So we just need to test the delegation, not all the ways where works.

Also, should probably just spell out that this is shorthand for where().delete/destroy_all.

@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch from fdf10ba to 784ff51 Feb 18, 2019
@abhaynikam
Copy link
Contributor Author

@abhaynikam abhaynikam commented Feb 18, 2019

@dhh : Cutoff most of the test cases and added some basic test case to check delegation. Also, updated the comments to mention shorthand for where().delete/destroy_all.

@dhh
Copy link
Member

@dhh dhh commented Feb 18, 2019

Looking good. Waiting for the builds to finish, but otherwise I believe good to merge.

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch 2 times, most recently from 6501353 to 3660957 Feb 18, 2019
@abhaynikam
Copy link
Contributor Author

@abhaynikam abhaynikam commented Feb 18, 2019

@kamipo : Fixed all suggestions. Thank You. 👍

# If no record is found, returns empty array.
#
# Person.all.destroy_by(name: 'Spartacus', rating: 4)
# Person.all.destroy_by("published_at < ?", 2.weeks.ago)

This comment has been minimized.

@kaspth

kaspth Feb 18, 2019
Member

This doesn’t seem to match @dhh’s description. I don’t think we want .all before a destroy_by. It should work like find_by straight on Person.


persons.destroy_by(id: [1, 2, 3])
persons.destroy_by(name: 'David')
persons.destroy_by(name: 'David', rating: 4)

This comment has been minimized.

@kaspth

kaspth Feb 18, 2019
Member

I think the plural of person is people (at the very least Rails’ inflector says so), so let’s go with that.

@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch 2 times, most recently from 75bbdc2 to acc507b Feb 18, 2019
@abhaynikam
Copy link
Contributor Author

@abhaynikam abhaynikam commented Feb 18, 2019

@kaspth : Done with the changes. Please have a look.

@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch 2 times, most recently from 21cbe58 to aa13107 Feb 18, 2019
@abhaynikam
Copy link
Contributor Author

@abhaynikam abhaynikam commented Feb 19, 2019

@kaspth : Fixed all the review comments.

cc/ @dhh @kamipo

assert_difference("Post.count", -3) { david.posts.destroy_by(body: "hello") }

destroyed = Author.destroy_by(id: david.id)
assert_equal [authors(:david)], destroyed

This comment has been minimized.

@abla00

abla00 Feb 19, 2019

It's easier to read:

assert_equal [david], destroyed

@abhaynikam abhaynikam force-pushed the abhaynikam:35304-add-delete_by_and_destroy_by branch from aa13107 to 5c8d4c3 Feb 19, 2019
@kamipo
kamipo approved these changes Feb 19, 2019
@kamipo kamipo merged commit 5c8d4c3 into rails:master Feb 19, 2019
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kamipo added a commit that referenced this pull request Feb 19, 2019
…troy_by

Introduce delete_by and destroy_by methods to ActiveRecord::Relation
@kamipo
Copy link
Member

@kamipo kamipo commented Feb 19, 2019

Thanks!

suketa added a commit to suketa/rails_sandbox that referenced this pull request Aug 17, 2019
Introduce delete_by and destroy_by methods to ActiveRecord::Relation
rails/rails#35316
@adamjubert
Copy link

@adamjubert adamjubert commented Nov 21, 2019

Is there any reason the name delete_by() was chosen as opposed to delete_where()? This nomenclature seems counterintuitive.

@dhh
Copy link
Member

@dhh dhh commented Nov 21, 2019

@goalaleo
Copy link

@goalaleo goalaleo commented Nov 26, 2019

I agree with @adamjubert . If destroy_by is basically just a delegator to

where(*args).destroy_all

then what do you call the method that delegates to

find_by(*args).destroy

Can't destroy_by and destroy_where both exist?

@dhh
Copy link
Member

@dhh dhh commented Nov 26, 2019

@goalaleo
Copy link

@goalaleo goalaleo commented Nov 28, 2019

destroy_where is an ugly method name to my eyes, breaks the link to the other _by methods, and adds nothing new not already present in the existing API.

I agree that it's not pretty. But what in your opinion is the link to the other _by methods?
If I saw the method name destroy_by for the first time, my intuition would tell me that it finds the first matching record, and calls .destroy on it. Don't all the other _by methods deal in singular records?

we are not returning any records

I'm not sure what we refers to here. Are you talking about the destroy_by method? If so, then doesn't it return all destroyed records ?

@westonganger
Copy link

@westonganger westonganger commented Dec 2, 2019

There is no point to "add missing symmetry" if the methods that are added do not symmetrically match the outcome, it only symmetrically matches the method name which is ridiculous.

I vote to remove these methods. The original solution of where().destroy_all or where().delete_all is much clearer. I don't ever want to have to deal with this code in a future project.

@dhh
Copy link
Member

@dhh dhh commented Dec 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants