-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Added a new usage-rules.md markdown document containing general rules… #4650
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.
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
base: master
Are you sure you want to change the base?
Conversation
… wor an LLM to follow when generating Ecto code.
usage-rules.md
Outdated
|
||
## TRANSACTION PATTERNS | ||
|
||
### Ecto.Multi for Complex Operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one might be better served with the newer Repo.transact
usage-rules.md
Outdated
with: [org_id: :org_id] # Composite foreign key | ||
end | ||
|
||
# ALWAYS scope queries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one will bump into the newish phoenix scoping stuff.
usage-rules.md
Outdated
posts |> Repo.preload([:author, :comments]) | ||
|
||
# AVOID nested preloads unless necessary | ||
# BAD: preload: [author: [posts: :comments]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily bad, even if you have N posts this will only be 3 queries. It's not ideal to do if you don't need those relations.
usage-rules.md
Outdated
3. **PERFORMANCE THIRD**: Prevent N+1 queries, use bulk operations when possible | ||
4. **CLARITY FOURTH**: Separate concerns, use multiple changesets, compose queries | ||
|
||
## COMMON LLM MISTAKES TO AVOID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Common Ecto LLM mistakes I see:
- unnecessary validations, ex: Ecto.Enum and validates_includes
- extra indexes all over with no query or usage pattern to justify
- LLM's changing migrations/schemas to fix related tests(lolsob)
- way over testing schema definitions (ie test to verify defaults, test to see of Ecto.Association is set on specific fields in the schema metadata, tests to verify repo.transaction works etc)
- using varchar instead of text
- putting size limits on text columns that was absurdly small via validations (based on nothing but vibes AFAICT)
- String.to_atom to change a Ecto.Enum into atom before cast
Thank you! This is a good beginning but I think it is too extensive. The models already know Ecto quite a bit, so I don't think our usage rules should go over API usage like upserts and similar. We should mostly focus on: security, new APIs (such as Repo.transact), and fixing bad practices/common mistakes LLMs may commit. |
I'd also suggest removing the "LLM" verbiage. I've seen no evidence that writing these docs as if you're writing it to or about an LLM has any positive impact. It's just "the usage rules" which are a condensed documentation with a target audience of an LLM. |
I have slimmed down the usage_rules.md and tried to keep only the essential. |
Purpose: Help coding agents understand the Ecto library and generate more accurate code following the established patterns and anti-patterns.
UsageRules is a development tool for Elixir projects that: