Add new "`delete_by` and `destroy_by`" rule

[koic]

trafficstars

This PR adds new delete_by and destroy_by rule.

Rails 6.0 has been added delete_by and destroy_by as relation methods. Prefer the use of delete_by over find_by(...)&.delete and where(...).delete_all, and destroy_by over find_by(...)&.destroy and where(...).destroy_all.

# bad
unreads.find_by(readable: readable)&.delete
unreads.where(readable: readable).delete_all

# good
unreads.delete_by(readable: readable)

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

# good
unreads.destroy_by(readable: readable)

[marcandre]

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.

[marcandre]

(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)

[marcandre]

Anyone interested can refer to the original issue: https://github.com/rails/rails/issues/35304 (there wasn't much support either)

[koic]

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 and find_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.

[pirj]

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.

[bbatsov]

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.

[pirj]

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.

`rg --no-ignore --hidden '\.delete_by|destroy_by'`

``` diaspora/spec/services/tag_following_service_spec.rb 41: expect(tag_following_service(alice).destroy_by_name(name)).to be_truthy 56: tag_following_service(alice).destroy_by_name(SecureRandom.uuid) 67: tag_following_service(alice).destroy_by_name("")

diaspora/spec/services/aspects_membership_service_spec.rb 54: aspects_membership_service.destroy_by_ids(@alice_aspect2.id, bob.person.id) 60: aspects_membership_service.destroy_by_ids(@alice_aspect2.id, eve.person.id) 67: aspects_membership_service.destroy_by_membership_id(@membership.id) 73: aspects_membership_service(eve).destroy_by_membership_id(@membership.id) 79: aspects_membership_service(eve).destroy_by_membership_id(-1) 87: aspects_membership_service.destroy_by_ids(@alice_aspect2.id, -1) 93: aspects_membership_service.destroy_by_ids(-1, eve.person.id) 99: aspects_membership_service(eve).destroy_by_ids(@alice_aspect2.id, bob.person.id)

diaspora/app/services/aspects_membership_service.rb 19: def destroy_by_ids(aspect_id, contact_id) 25: def destroy_by_membership_id(membership_id)

diaspora/app/services/tag_following_service.rb 31: def destroy_by_name(name)

diaspora/app/controllers/aspect_memberships_controller.rb 14: delete_results = AspectsMembershipService.new(current_user).destroy_by_membership_id(params[:id])

diaspora/app/controllers/api/v1/contacts_controller.rb 46: result = aspects_membership_service.destroy_by_ids(aspect_id, person.id) if person.present?

diaspora/app/controllers/api/v1/tag_followings_controller.rb 28: tag_followings_service.destroy_by_name(params.require(:id))

sharetribe/spec/models/listing_spec.rb 160: Listing.delete_by_author(author.id)

sharetribe/app/services/admin2/membership_service.rb 88: Listing.delete_by_author(person.id)

sharetribe/app/services/admin/communities/membership_service.rb 94: Listing.delete_by_author(person.id)

sharetribe/app/controllers/people_controller.rb 221: Listing.delete_by_author(target_user.id)

sharetribe/app/models/listing.rb 382: def self.delete_by_author(author_id)

discourse/lib/email/cleaner.rb 20: IncomingEmail.delete_by('rejection_message IS NOT NULL AND created_at < ?', SiteSetting.delete_rejected_email_after_days.days.ago)

lobsters/app/models/hat.rb 21: def destroy_by_user_with_reason(user, reason)

forem/lib/data_update_scripts/20210518043957_remove_site_config_scripts.rb 14: DataUpdateScript.delete_by(file_name: SCRIPTS_TO_REMOVE)

forem/lib/data_update_scripts/20210512032437_remove_settings_data_update_scripts.rb 16: DataUpdateScript.delete_by(file_name: SCRIPTS_TO_REMOVE)

forem/lib/data_update_scripts/20220203143904_remove_logo_svg_from_database.rb 4: Settings::General.destroy_by(var: :logo_svg)

forem/lib/data_update_scripts/20220214195145_remove_profile_field_update_scripts.rb 15: DataUpdateScript.delete_by(file_name: FILE_NAMES)

forem/lib/data_update_scripts/20210701134702_remove_notification_setting_migration_scripts.rb 9: DataUpdateScript.delete_by(file_name: SCRIPTS_TO_REMOVE)

forem/lib/data_update_scripts/20210512030719_mark_mascot_setting_dus_as_obsolete.rb 4: DataUpdateScript.delete_by(file_name: "20210504060704_move_mascot_settings_backto_site_config")

forem/lib/data_update_scripts/20210604104735_remove_email_addresses_scripts.rb 4: DataUpdateScript.delete_by(file_name: "20210509105151_remove_unused_site_config_emails") 5: Settings::General.delete_by(var: :email_addresses)

forem/lib/data_update_scripts/20210503202046_remove_unused_data_update_scripts.rb 44: DataUpdateScript.delete_by(file_name: FILE_NAMES)

forem/lib/data_update_scripts/20210712054300_remove_unused_profile_fields.rb 22: ProfileField.destroy_by(attribute_name: OBSOLETE_FIELDS)

forem/lib/data_update_scripts/20211206222716_remove_stackbit_page.rb 4: Page.destroy_by(slug: "connecting-with-stackbit")

forem/app/services/articles/updater.rb 41: ContextNotification.delete_by(context_id: article.id, context_type: "Article",

forem/app/controllers/profile_pins_controller.rb 21: current_user.profile_pins.destroy_by(id: params[:id])

forem/app/workers/html_variants/remove_old_data_worker.rb 8: HtmlVariantTrial.delete_by("created_at < ?", 2.weeks.ago) 9: HtmlVariantSuccess.delete_by("created_at < ?", 2.weeks.ago)

mastodon/db/migrate/20200508212852_reset_unique_jobs_locks.rb 8: until SidekiqUniqueJobs::Digests.new.delete_by_pattern('*').nil?; end

gitlabhq/db/post_migrate/20210420103955_remove_hipchat_service_records.rb 14: relation.delete_by(type: 'HipchatService')

gitlabhq/spec/lib/gitlab/avatar_cache_spec.rb 65: subject { described_class.delete_by_email(*emails) }

gitlabhq/spec/models/integration_spec.rb 269: Integration.delete_by(**integration_hash(:asana))

gitlabhq/app/models/concerns/cache_markdown_field.rb 179: user_mention_class.delete_by(identifier)

gitlabhq/app/models/concerns/avatarable.rb

</details>

[koic]

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!