"Redundant `all`" is too naive about rails-style-guide HOT 10 OPEN

My only point was that suggesting (in the style guide and cop rule) that .all can safely be removed from every statement is very dangerous. In my case, the change was caught by non-null violation (and it still took me quite some time to understand what happened), but I might imagine somebody's code's behavior changing without them noticing.

So, at the very least:

• style guide should make a clear statement that behavior with and without .all most probably is different for associations;
• I honestly don't know what's the sane behavior for a cop. I don't know how many codebases are actually relying on model.association.delete_all (or, as ours did, on model.association.all.delete_all). If it were up to me I'd say the rule that doesn't work in 50% of its possible usages is not a rule at all 🤷

PS: On a more philosophical note, there could be a lot of discussion around CollectionProxy design (which sometimes has the same behavior as a naked scope, and sometimes has completely different with same method names, and it all "makes sense in context" until you work in a big production codebase or follow the wrong assumption), but that's just the reality we have 🤷

from rails-style-guide.

cc @masato-bkn

from rails-style-guide.

Hey Victor!

Interesting case with delete_all indeed. But how is the styleguide/cop responsible for such an inconsistent and surprising behavior in Rails?
Without any cop or a guideline, wouldn’t you anyway remove the ‘.all’ part from that statement?

I wouldn’t be surprised if it was a many to many association, like ‘peter.friends.delete_all’. But it would remove records from the join table, not nullify, wouldn’t it?

I’d stick to “no .all unless it’s directly called on the model” and let Rails reconsider its default that goes against the principle of least surprise.

Wondering what actions would you guys take to make everyone’s lifes easier?

from rails-style-guide.

The example ‘person.pets.delete(Pet.find(1))’ looks quite weird. I would probably write it differently, but this notation deserves to exist.

In contrast to the surprising nullify for delete_all on has_many, this is a real dilemma. Pets can become orphans, and another person can adopt it. But an article, a doctor appointment, a diary note?
How would Rails know the semantic? They had to pick a default, and they chose a safer one that doesn’t remove records. And that would fail on the DB level with proper FK constraints if the AR mapping is broken, or someone accidentally removes the ‘.all’.

To this moment I was trying to avoid ‘dependent:’ as much as possible, and to rely on DB consteaints instead (‘add_foreign_key … on_delete: :cascade’.
One reason was that one of my past projects had such a threaded record structure that deleting a user could take longer than an hour.

However, your example shows that onitting ‘dependent:’ isn’t always a good idea.

Do you think it makes sense to require to specify ‘dependent:’ on every has_many/has_many through?

from rails-style-guide.

can safely be removed from every statement is very dangerous

It’s a very good point. I doubt that the guide is providing guarantees that switching from a “bad” style to a “good” style is always safe and has no side effects. Take, for example, the load Rails config defaults rule. Blindly turning the loading on can have disastrous effects, not all of which can be caught by specs or on a low-load few-data staging server.

Speaking if the cop, it should be marked as unsafe for autocorrection.

from rails-style-guide.

A practical clarification. Why wasn’t the change from ‘user.articles.all.delete_all’ to ‘user.articles.delete_all’ detected? I recon it should have been caught by a not null database constraint on ‘articles.user_id’ column?

from rails-style-guide.

I doubt that the guide is providing guarantees that switching from a “bad” style to a “good” style is always safe and has no side effects.

That's true. But I believe that at least mentioning the fact about possible change of the semantics should be considered 🤷
Though, thinking a bit further of it, it seems that in the wild only delete_all and similar methods will be affected. So that might be the point to mention in the guide/fix in cop (checking for those specific methods).

Why wasn’t the change from user.articles.all.delete_all to user.articles.delete_all detected?

It was caught by the test immediately, but I spent some time trying to understand why delete_all behavior changed 😁 (And in some codebases there might be no DB-level constraints to caught that.)

from rails-style-guide.

Thank you for pointing that out.

Considering that the behavior of some methods changes when the receiver is a associations, it might be better to limit this style guide and cop to situations where the receiver is a Model🤔

When the receiver is a Model, at least for methods in ActiveRecord::Querying::QUERYING_METHODS, they are delegated to all. So, I think omitting all does not change the behavior.

from rails-style-guide.

The guideline can make a note regarding the ‘delete’/‘delete_all’ and the default ‘dependent’.
It would still tell to not use the ‘.all’, in all cases.
But it, ot another guideline/cop would could tell to use ‘destroy’/‘destroy_all’ instead.

Do you think this would promote the style that has the least surprise?

from rails-style-guide.

Do you think this would promote the style that has the least surprise?

OK, I generally agree with you!

The guideline can make a note regarding the ‘delete’/‘delete_all’ and the default ‘dependent’.

At present, this cop checks for ActiveRecord::Querying::QUERYING_METHODS.
Regarding delete, it's not targeted by this cop, so I'm uncertain whether we need to mention it in the note for this style guide.

Also, for destroy_all, since its behavior changes depending on whether all is present or not, I think a note is necessary.

from rails-style-guide.