Fix AGENTS.md including Ecto references in --no-ecto projects #6581
Conversation
SteffenDE
left a comment
SteffenDE
left a comment
There was a problem hiding this comment.
I don't think we should go ahead with this as is, see comments below.
usage-rules/ecto.md
Outdated
| - Fields which are set programatically, such as `user_id`, must not be listed in `cast` calls or similar for security purposes. Instead they must be explicitly set when creating the struct | ||
| - **Always** invoke `mix ecto.gen.migration migration_name_using_underscores` when generating migration files, so the correct timestamp and conventions are applied | ||
|
|
||
| ### Creating LiveView forms from changesets |
There was a problem hiding this comment.
This only moves the problem around. Now, you'd get LiveView content when you use --no-live. For the AGENTS.md, we could solve this by using .md.eex files and then checking <%= if live do %> etc., but since these files are also valid usage_rules, so we can't use eex :(
Maybe we need to split this even further, into an ecto-liveview.md and concat that conditionally? Although this refers to LiveView, the content is also valid for Phoenix forms without LiveView.
usage-rules/elixir.md
Outdated
|
|
||
| - **Never** nest multiple modules in the same file as it can cause cyclic dependencies and compilation errors | ||
| - **Never** use map access syntax (`changeset[:field]`) on structs as they do not implement the Access behaviour by default. For regular structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist, `Ecto.Changeset.get_field/2` for changesets | ||
| - **Never** use map access syntax on structs as they do not implement the Access behaviour by default. For structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist |
There was a problem hiding this comment.
| - **Never** use map access syntax on structs as they do not implement the Access behaviour by default. For structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist | |
| - **Never** use map access syntax on structs (`my_struct[:field]`) as they do not implement the Access behaviour by default. For structs, you **must** access the fields directly, such as `my_struct.field` or use higher level APIs that are available on the struct if they exist |
usage-rules/liveview.md
Outdated
| <.form for={@data} id="my-form"> | ||
| <.input field={@data[:field]} type="text" /> |
There was a problem hiding this comment.
I don't think this change is good, as nobody would write that, and it probably doesn't really get the point across to the LLM.
|
I agree with your comment @SteffenDE - even with --no-ecto we still have Ecto references. It's more complex to create the AGENTS.md for all phx.new flags. But keeping Ecto references in non-Ecto Phoenix projects throws off the Agents. Happy to spend some time looking into all cases if we want an AGENTS.md for all scenarios. |
bmalum
force-pushed
the
fix-agents-md-ecto-references
branch
from
a428483 to
95c4c8b
Compare
| "<!-- phoenix:ecto-live-forms-start -->\n", | ||
| @rules_files["ecto-live-forms.md"], | ||
| "\n<!-- phoenix:ecto-live-forms-end -->" | ||
| ], |
There was a problem hiding this comment.
While this works for phx.new, it doesn't work for mix usage_rules.sync, since you then get basically the same content twice :(
So I don't see a good way to make this work without lots of duplication (basically having one set of rules for the root usage-rules and then a separate set of files for the generator). The question then is if we're fine with that and we want to strive for perfect AGENTS.md for all possible phx.new flags, or if we instead explicitly only support the default case. Then, we'd tell people to use --no-agents-md or we could also skip generating the AGENTS.md file for non-trivial flags to not generate confusing content. I don't know... (cc @chrismccord @josevalim)
There was a problem hiding this comment.
@SteffenDE I think you can control in usage_rules what you want to sync, no?
In any case, I think this is changing too much at the same time. For example, I would not be inclined to split the Phoenix one into multiple sections. But I am fine with two versions for Ecto (as long as we confirm usage_rules can choose what to sync).
So in my two cents:
- changes to elixir.md is good
- changes to ecto.md is good
- having two Ecto guides is fine (as long as we confirm usage_rules can be customised)
| - Phoenix templates **always** use `~H` or .html.heex files (known as HEEx), **never** use `~E` | ||
| - **Always** use the imported `Phoenix.Component.form/1` and `Phoenix.Component.inputs_for/1` function to build forms. **Never** use `Phoenix.HTML.form_for` or `Phoenix.HTML.inputs_for` as they are outdated | ||
| - When building forms **always** use the already imported `Phoenix.Component.to_form/2` (`assign(socket, form: to_form(...))` and `<.form for={@form} id="msg-form">`), then access those forms in the template via `@form[:field]` | ||
| - When building forms **always** use the already imported `Phoenix.Component.to_form/2` (`assign(:form, to_form(...))` and `<.form for={@form} id="msg-form">`), then access those forms in the template via `@form[:field]` |
There was a problem hiding this comment.
Phoenix.Component already assumes LiveView, so this change still assumes LiveView. I would keep it as is, I think the templates one below is fine.
3 participants
When generating a new Phoenix project with
--no-ecto, the AGENTS.md file still contained references to Ecto (changesets, schemas, etc.). This confused AI agents working on these projects since they'd see Ecto guidelines, but the project doesn't actually use Ecto.--no-ecto.Please feel free to comment with ideas on how to better handle the
<.form for={@changeset} id="my-form">to<.form for={@data} id="my-form">change. Ideally we'd keep changeset info for Ecto projects but use generic data examples for non-Ecto Phoenix apps.