Remove rules that can be replaced with a linter.#792
Conversation
In an effort to follow [best practices], we remove items that can be enforced with a linter. The spirit of this commit is to keep CLAUDE.md as lean as possible, and relates to this [feedback]. File | Removed rule | Cop / Tool | |---|---|---| | controllers.md | Actions should not exceed 10 lines | `Metrics/MethodLength` (built-in, default `Max: 10`) | | controllers.md | Never `params.permit!` (kept in security.md) | No built-in RuboCop cop. Brakeman flags it; custom cop needed for lint coverage. `Rails/StrongParametersExpect` targets `params.require(...).permit(...)` → `params.expect(...)`, **not** `permit!`. | | database.md | Migrations must be reversible | `Rails/ReversibleMigration` (rubocop-rails) | | database.md | Add `null: false` and database-level defaults where appropriate | Partial only. `Rails/NotNullColumn` flags `add_column ..., null: false` *without* a default (a related gotcha, not the same rule). `Rails/ThreeStateBooleanColumn` enforces `null: false` on boolean columns specifically. No general cop enforces `null: false` across all columns. | | models.md | Long parameter lists (max 3 args) | `Metrics/ParameterLists` (built-in, set `Max: 3`) | | models.md | Extract models >200 lines / 15 public / 7 private methods | `Metrics/ClassLength` covers the 200-line threshold only. No built-in cop for public/private method counts — custom cop required. | | testing.md | One `expect` per `it` block | `RSpec/MultipleExpectations` (rubocop-rspec) | | testing.md | Max 2 levels of context nesting | `RSpec/NestedGroups` (rubocop-rspec, set `Max: 2`) [best practices]: https://code.claude.com/docs/en/best-practices#write-an-effective-claude-md [feedback]: thoughtbot/readysetgo#190 (comment)
dae1167 to
19b25db
Compare
JoelQ
left a comment
There was a problem hiding this comment.
💯 to the spirit of this change. IMO, most style and a lot of best practices don't belong in CLAUDE.md
Not sure where we should make additional changes so we don't lose this idea? Should these cops be added to suspenders? Should we document some of these things so we don't lose the preferences?
That's my plan 😎. I want to ship this first, since we're already pulling these rules into Suspenders. Then, I plan on introducing some Rubocop tooling. |
|
What if we have a guide itself advice with agents/AI, etc. that way we can use the language of the guides. ie,
|
jaredlt
left a comment
There was a problem hiding this comment.
Very much in favour of "moving things left", and where we can strictly enforce something with linting we should. I left some comments about some of the more "it depends" rules that I feel might still be useful as hints to the LLMs.
| # Controllers | ||
|
|
||
| - Controllers handle HTTP only: receive request, delegate to model, return response. | ||
| - Actions should not exceed 10 lines (excluding strong params). Longer actions often signal business logic that belongs in a model or PORO. |
There was a problem hiding this comment.
The "should not" here is a hint to not make long actions. As opposed to "must not" which is a hard rule.
I worry about enforcing this one with lint rules because there are always exceptions.
I also wonder if we lose some context for the agent when we remove this, especially the second sentence "Longer actions often signal business logic that belongs in a model or PORO."
There was a problem hiding this comment.
I worry about enforcing this one with lint rules because there are always exceptions.
That's actually the perspective I'm taking here, but in this case, the author would be responsible for disabling the rule with a comment.
My thinking is there are always exceptions, which is why RuboCop allows rules to be disabled.
There was a problem hiding this comment.
Would you be comfortable with something like :
Avoid long actions, since they often signal business logic that belongs in a model or PORO.
|
|
||
| - Always use the `rails generate migration` command to create migration files. | ||
| - Migrations must be reversible. | ||
| - Add `null: false` and database-level defaults where appropriate. |
There was a problem hiding this comment.
I think this is a nice hint to the LLMs to prefer null: false where appropriate because as you mention in your description, there is no hard rule for this. It depends.
There was a problem hiding this comment.
@jaredlt we should be covered by Rails/NotNullColumn.
There was a problem hiding this comment.
Sorry, I should elaborate. I like the idea of having the linter bring this to the attention of the author just to be safe. They can always decide to add a comment disabling it in that migration.
| - Factories: only required attributes with sensible defaults. Start in `spec/factories.rb`. | ||
| - Shoulda Matchers for validations and associations. | ||
| - WebMock blocks all external HTTP in tests — always stub external requests. | ||
| - One `expect` per `it` block. Max 2 levels of context nesting. |
There was a problem hiding this comment.
I'm fine with removing this one but I do wonder about enforcing the lint rules. Sometimes an extra expect in the same test tells the story better. Sometimes an extra context wrapped around a test tells the story better.
| - Migrations must be reversible. | ||
| - Add `null: false` and database-level defaults where appropriate. | ||
| - Use `text` over `string` if length varies significantly. | ||
| - Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions. |
There was a problem hiding this comment.
Can the direction to use save! also be removed? Rubocop should flag use of .save or .update anywhere, whether or not in a transaction:
| - Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions. | |
| - Wrap multi-record operations in transactions. |
There was a problem hiding this comment.
I don't know enough about RuboCop here but aren't there valid reasons why you wouldn't want to use the bang version of these methods? If we enforce that via lint then do we need to add exclusions everywhere we don't want them?
There was a problem hiding this comment.
There are valid reasons why you wouldn't want to use the bang version of a method, and the one I've run across the most is because you're calling some library that does not have a bang method defined for the thing you need to call.
For this PR, removing the guidance from CLAUDE.md simplifies the LLM's task and pushes it to the linter and the human, in which case you add an exception if necessary.
louis-antonopoulos
left a comment
There was a problem hiding this comment.
I looked through all the full files in the PR and only found one tiny additional opportunity for reduction.
Looks good!
|
I share the same comments from @jaredlt. I think some of the rules are very useful for the LLM to generate better code in the very first prompt without having to rectify it later with another prompt or run the linter, especially in "it depends" situations like I think this type of rule or hint is quite useful. Or at least I have found that it enforces this a lot thanks to the rule. |
In an effort to follow best practices, we remove items that can be
enforced with a linter.
The spirit of this commit is to keep CLAUDE.md as lean as possible, and
relates to this feedback.
Metrics/MethodLength(built-in, defaultMax: 10)params.permit!(kept in security.md)Rails/StrongParametersExpecttargetsparams.require(...).permit(...)→params.expect(...), notpermit!.Rails/ReversibleMigration(rubocop-rails)null: falseand database-level defaults where appropriateRails/NotNullColumnflagsadd_column ..., null: falsewithout a default (a related gotcha, not the same rule).Rails/ThreeStateBooleanColumnenforcesnull: falseon boolean columns specifically. No general cop enforcesnull: falseacross all columns.Metrics/ParameterLists(built-in, setMax: 3)Metrics/ClassLengthcovers the 200-line threshold only. No built-in cop for public/private method counts — custom cop required.expectperitblockRSpec/MultipleExpectations(rubocop-rspec)RSpec/NestedGroups(rubocop-rspec, setMax: 2)