Closed koic closed 1 year ago
I'll be a contrarian (again) and say that introducing delete_by
and destroy_by
was a mistake. There is a cost of introducing a method (besides added maintenance): it augments cognitive load.
There is no difference between where(...).delete_all
and delete_by
, so there should have been a burden of proof that where(...).delete_all
is frequent. In my limited experience, that is not the case.
The argument "by symmetry" is not sufficient and isn't even relevant as find_by
and find_or_create_by
apply to a single record, not multiple records.
Moreover, find_by(readable: readable)&.delete
is not equivalent to delete_by(readable: readable)
when there is more than more than 1 readable record.
In short, I am against a rule that forces people to learn and remember the existence of a nearly useless and doubtfully named method.
(I would even approve the contrary rule: do not use delete_by
/destroy_by
as they are obscure, error prone and confusing, but I'm not expecting everyone would agree with this)
Anyone interested can refer to the original issue: https://github.com/rails/rails/issues/35304 (there wasn't much support either)
Thank you for sharing your opinion.
These APIs were proposed by DHH and I think it is unlikely that it may change in future. https://github.com/rails/rails/pull/35316#issuecomment-560527408
The argument
"by symmetry"
is not sufficient and isn't even relevant as find_by andfind_or_create_by
apply to a single record, not multiple records.
I use the APIs in real world Rails 6.0 application. Before I used it, I was concerned about the symmetry of API design to existing find_by
series, but after using it, the hurdle is lower than I expected.
However, I understand that style has the opposite view. I would like to pending this PR for a while and see what happens for other opinions.
The promise of getting rid of safe navigation in:
unreads.find_by(readable: readable)&.destroy
personally appeals to me, but I stick with Marc-André that the cognitive burden cost isn't worth it.
Scarce Reddit comments in their overwhelming majority have negative attitude towards those new methods.
I'd love to see some real-world usages and hear what the community thinks.
Side note: from Rails PR thread I've learned that find_all_by
exists. Have never seen it used in the wild.
One year later - what are we doing about this? That's another area where I don't have a strong preference, so I'm fine either way.
There are quite a few real-world usages (checked against real-world-rspec
, can be used even more if checked against real-world-rails
).
A good half of examples is delete_by_*
, not a plain delete_by
. Does it make sense to reflect this in the guideline?
I'd check against real-world-rails
to see more real usages. The reason I'm suggesting this is:
A style guide that reflects real-world usage gets used, while a style guide that holds to an ideal that has been rejected by the people it is supposed to help risks not getting used at all - no matter how good it is.
This is a matter of differing opinions or preferences, and due to the lack of recent activity, I'm going to close this issue. Thank you for the discussion and investigating!
This PR adds new
delete_by
anddestroy_by
rule.Rails 6.0 has been added
delete_by
anddestroy_by
as relation methods. Prefer the use ofdelete_by
overfind_by(...)&.delete
andwhere(...).delete_all
, anddestroy_by
overfind_by(...)&.destroy
andwhere(...).destroy_all
.