Skip to content

Remove rules that can be replaced with a linter. #792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stevepolitodesign wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove rules that can be replaced with a linter. #792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19b25db
Remove rules that can be replaced with a linter.
stevepolitodesign May 14, 2026
e7be045
Address feedback
stevepolitodesign May 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions rails/ai-rules/rules/controllers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# 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.
Comment thread
jaredlt marked this conversation as resolved.
- Avoid long actions, since they often signal business logic that belongs in a model or PORO.
- Maximum one instance variable per action.
- No business logic, calculations, email sending, or multi-object operations in controllers.
- Always use strong parameters. Never `params.permit!`.
- Return `status: :unprocessable_entity` on failed form renders (required by Turbo).
- Prefer RESTful routes. Custom verb actions (e.g., post "activate") usually mean a missing noun/resource (e.g., resource :trial, only: [:create]).
2 changes: 0 additions & 2 deletions rails/ai-rules/rules/database.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# Database & Migrations

- Always use the `rails generate migration` command to create migration files.
- Migrations must be reversible.
- Add `null: false` and database-level defaults where appropriate.
Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@stevepolitodesign stevepolitodesign May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaredlt we should be covered by Rails/NotNullColumn.

Copy link
Copy Markdown
Contributor Author

@stevepolitodesign stevepolitodesign May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I agree with this. I do think we need to experiment with what works for the linter. We want it to be right most of the time rather than feel like we're fighting it.

That cop doesn't help remind us that ideally we want to use null: false for most columns. It only triggers if an add_column has null: false without a default as well. So if you just add_column without a null: false it will succeed.

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevepolitodesign would you be open to keeping the null: false line? My understanding is we should have a good reason to not add null: false (we typically want to add it or ask ourselves why we don't want it).

My reading of that cop is that it doesn't enforce us to using null: false. I think I'd prefer to have the LLM try to remember to use null: false rather than omit it and we end up introducing a less strict data model.

I'm also open to this being enforced via lint the future with some form of opt out. I just don't know what that looks like exactly yet.

I think the rest of the PR looks great.

- Use `text` over `string` if length varies significantly.
- Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions.
Copy link
Copy Markdown
Contributor

@louis-antonopoulos louis-antonopoulos May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the direction to use save! also be removed? Rubocop should flag use of .save or .update anywhere, whether or not in a transaction:

Suggested change
- Wrap multi-record operations in transactions. Use `save!` (bang) inside transactions.
- Wrap multi-record operations in transactions.

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@louis-antonopoulos louis-antonopoulos May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surfacing errors is a very common case for non-bang save/update methods. I'm worried about us scattering our codebases with exceptions for this very common case. This goes to my point about wanting to ensure we don't fee like we're fighting the linter

Copy link
Copy Markdown
Contributor

@louis-antonopoulos louis-antonopoulos May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. I might be misunderstanding here -- since bang methods raise, shouldn't we encourage those in the linter to surface errors?

  2. Using save! in a transaction in a transaction is something that should be happening across the board, because a save returning false will not cause the transaction to be rolled back, leading to data integrity issues. This should be set at the linting level to make sure humans and LLMs don't do something like forget a bang method, and end up reducing one bank account balance during a funds transfer but failing to increment the recipient's balance because part of the transaction returned false instead of raising.

The goal is to enforce rules at an automatic level and let a human decide about exceptions, not rely on an agent to remember a hint for something that could be catastrophic.

So the change here is don't give hints to the agent where an automatic tool will enforce things. Give hints to the agent where the automatic tool cannot guide it.

Copy link
Copy Markdown
Contributor

@JoelQ JoelQ May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have LLMs the cost of creating fancier lint rules has gone down a lot. I could imagine a lint rule that requires save! inside transactions or even one that requires save! if you aren't using the return value.

- Keep scopes as one-liners. Complex queries belong in search/query objects.
Expand Down
5 changes: 2 additions & 3 deletions rails/ai-rules/rules/models.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
- Name classes after domain nouns, not actions. No `*Service`, `*Manager`, `*Handler` suffixes.
- Use `ActiveModel::Model` for POROs that need validation or form integration.
- Replace `.call` / `.perform` with domain verbs: `#save`, `#complete`, `#submit`, `#deliver`.
- Look to identify domain models that can be extracted when an existing
Comment thread
stevepolitodesign marked this conversation as resolved.
model exceeds: 200 lines, 15 public methods, or 7 private methods.
- Look to identify domain models that can be extracted when an existing model is large.
- Callbacks only for data integrity (normalise fields, set defaults). Never for emails, payments, or external systems.
- Prefer composition over inheritance. Extract behaviour into small, focused objects.
- Avoid feature envy, long parameter lists (max 3 args), case statements on type, and mixin abuse.
- Avoid feature envy, long parameter lists, case statements on type, and mixin abuse.
1 change: 0 additions & 1 deletion rails/ai-rules/rules/testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,4 @@
- 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.
Copy link
Copy Markdown
Contributor

@jaredlt jaredlt May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- Never test private methods directly. Never stub the system under test.
Loading