Skip to content
New issue

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

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

Already on GitHub? # to your account

Adding new delete_all query method to remove records based on query #169

Merged
merged 5 commits into from
Jul 26, 2019
Merged

Adding new delete_all query method to remove records based on query #169

merged 5 commits into from
Jul 26, 2019

Conversation

jwoertink
Copy link
Member

Fixes #111

Based on how rails works, when you run the delete_all method, it'll return an integer, so it's going to be the last thing you run in your query. You can chain all your wheres like normal though.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was actually thinking about this yesterday and how “delete_all” is kind of confusing. Is it deleting all or just a subset?

What do you think about “bulk_delete” or “mass_delete” instead? That way it doesn’t mislead people into thinking it’ll just delete everything

spec/query_spec.cr Outdated Show resolved Hide resolved
src/avram/queryable.cr Outdated Show resolved Hide resolved
@jwoertink
Copy link
Member Author

hmm.... I can see how it may be a bit confusing, though, I do feel like delete_all is pretty standard convention.

Ecto: https://hexdocs.pm/ecto/Ecto.Repo.html#c:delete_all/2
Rails: https://api.rubyonrails.org/classes/ActiveRecord/Relation.html#method-i-delete_all

Though, some do break that
Gorm: http://gorm.io/docs/delete.html#Batch-Delete uses Delete
Laravel: http://gorm.io/docs/delete.html#Batch-Delete uses ->delete()
Granite: https://github.com/amberframework/granite/blob/c4bc435fc9a24a132ff349367ba2b324d22d8948/docs/crud.md#delete not sure if they have a delete based on WHERE query, but maybe clear?
Django: https://docs.djangoproject.com/en/dev/ref/class-based-views/#deleteview also uses delete...

so maybe we just call it delete?

@paulcsmith
Copy link
Member

Good call. I think delete makes sense. Other orms use it. It’s what the SQL call uses. I like it!

@paulcsmith
Copy link
Member

Also I think we should add delete_all and do a compile time raise that tells people to use delete.

That way if people use it they’ll know what to do

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge nice those last bits are addressed

src/avram/queryable.cr Outdated Show resolved Hide resolved
src/avram/queryable.cr Outdated Show resolved Hide resolved
@jwoertink jwoertink merged commit 1cb8c57 into luckyframework:master Jul 26, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add delete_all that deletes all based on the query
2 participants