Ban rails default_scope usage
Description of the proposal
This has come up twice in two days in the #backend-maintainers
channel:
- https://gitlab.slack.com/archives/C8HG8D9MY/p1591090512274200 (re: !31757 (comment 353395935) )
- https://gitlab.slack.com/archives/CJHPLRTRD/p1591268880232600 (re: !33498 (comment 353697278) )
My opinion is that this AR method does more harm than good, especially in large codebases:
- It's very unlikely that the default scope will be relevant for all cases, so you'll need to
unscope
sometimes - Changing it once it's established is very difficult, since only some sites declare their requirements
- Everyone is going to be surprised by it every time they use the model
https://github.com/rubocop-hq/rubocop-rails/issues/76 lists these blog posts against it:
- https://rails-bestpractices.com/posts/2013/06/15/default_scope-is-evil/
- https://andycroll.com/ruby/dont-use-default-scope/
- https://stackoverflow.com/questions/25087336/why-is-using-the-rails-default-scope-often-recommend-against/
The existing list of uses is small, so I've added disable-comments for them in this MR. I know we had a large effort to remove default scopes from Project
and other large models in the past. I think it's worth preventing any more from coming in now.
Check-list
-
Make sure this MR enables a static analysis check rule for new usage but ignores current offenses -
Mention this proposal in the relevant Slack channels (e.g. #development
,#backend
,#frontend
) -
The MR doesn't have significant objections, and is getting a majority of 👍 vs👎 (remember that we don't need to reach a consensus) -
Create a follow-up issue to fix the current offenses as a separate iteration: #220817 -
Follow the review process as usual -
Once approved and merged by a maintainer, mention it again: -
In the relevant Slack channels (e.g. #development
,#backend
,#frontend
) -
(Optional depending on the impact of the change) In the Engineering Week in Review
-
/cc @gitlab-org/maintainers/rails-backend
cc @manojmj @minac as authors of the two referenced MRs above.
Edited by Nick Thomas