Add RuboCop cop to enforce no strict loading model or association config #69

dkubb · 2025-01-18T00:47:15Z

Summary

Description

This branch adds two cops to ensure models and associations don't have strict loading configuration.

* Add ByDefaultForModels to ensure models do not set self.strict_loading_by_default. * Add ForAssociations to ensure model associations do not set :strict_loading option. The purpose of this cop is to make sure to highlight any places where we explicitly set strict loading, as opposed to relyong on global settings. The idea will be to capture all the offenses in a .rubocop_todo.yml so we can burn down and fix all the cases over time.

dkubb · 2025-01-19T18:50:27Z

.github/workflows/tests.yml

@@ -8,7 +8,7 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        ruby: [3.0, 3.1, 3.2]
+        ruby: ['3.0', 3.1, 3.2]


comment: This fixes a known issue with GitHub Actions.

lib/rubocop/cop/betterment/no_strict_loading.rb

dkubb · 2025-01-21T19:17:49Z

spec/rubocop/cop/betterment/use_global_strict_loading/for_associations_spec.rb

+      expect_offense(<<~RUBY)
+        class MyModel < ApplicationRecord
+          has_many :posts,        # preserve this comment
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not set `:strict_loading` in ActiveRecord associations.


comment: This was a bit annoying that I couldn't figure out how to make it also highlight the actual option on the next line. However, I feel it's probably "close enough" that anyone could figure out that the problem is associated with this specific has_many call.

For this cop to work the auto-correct is not strictly required; and I should note the autocorrect does work in this case, the problem is mostly wrt what parts are highlighted, which is a secondary concern.

rzane · 2025-01-21T19:24:58Z

Should we register these cops in config/default.yml and specify SafeAutoCorrect: false? Performing these autocorrections are very likely to introduce new errors that need to be manually resolved.

dkubb · 2025-01-21T19:40:44Z

@rzane

Should we register these cops in config/default.yml and specify SafeAutoCorrect: false?

comment: I saw somewhere that you can define def safe_autocorrect? = false in the cop. I wonder if that would be even better? It does appear as if we have a convention of using SafeAutoCorrect: false so perhaps that is something I should do also/instead?

dkubb · 2025-01-21T19:45:07Z

comment: For now I've defined SafeAutoCorrect: false in default/config.yml and also specified it in the cop source, since the default for #safe_autocorrect? is true, which seems like an unsafe default TBH.

lib/rubocop/cop/betterment/use_global_strict_loading.rb

rzane

domain lgtm

smudge

domain LGTM && platform LGTM - thanks for the README update!

smudge

domain LGTM!

dkubb added 3 commits January 17, 2025 14:50

Fix GitHub Action to quote ruby 3.0 version so it's not evaluated as 3

98706ef

Fix rubocop/cop/betterment require list to be sorted

e2784ab

dkubb force-pushed the dkubb/add/activerecord-strict-loading-cops branch 5 times, most recently from 4e4c855 to 2c2ad8a Compare January 19, 2025 18:48

dkubb commented Jan 19, 2025

View reviewed changes

dkubb force-pushed the dkubb/add/activerecord-strict-loading-cops branch 2 times, most recently from 0f03e68 to 766f0db Compare January 19, 2025 19:05

dkubb marked this pull request as ready for review January 19, 2025 19:12

dkubb force-pushed the dkubb/add/activerecord-strict-loading-cops branch from 766f0db to 2f6406c Compare January 19, 2025 20:06