improvement: add rename_module/4 and mix igniter.refactor.rename_modu…#374
Conversation
zachdaniel
left a comment
There was a problem hiding this comment.
Looking mostly
Good have a few comments!
Use find_module/2 and AST-based multi-alias detection instead of string-matching helpers. Fix an ordering bug where the AST pass ran before the string replace (Example → NewExample → NewNewExample). Add import/require tests.
|
Thanks for the review! Pushed an update:
What I already started before your comment:
Tests: |
- Replace custom find_module_file with Igniter.Project.Module.find_module/2
- Replace string-based multi_alias_in_content? with AST zipper traversal
- Fix double-rename bug (NewNewExample) by running string replace before AST pass
- Fix no-source crash by guarding move_module_file with Rewrite.source/2 check
- Fix alias Ns.{FooPage, BarPage} collapsing to alias Ns.{IndexLive, IndexLive}
when both members move to different namespaces but share the same short name;
expand_multi_alias_member now splits the multi-alias into separate alias statements
|
Fixed another bug in multi-alias handling: when renaming two LiveView pages from a shared namespace to separate ones — e.g. alias MyApp.{FooLive, BarLive} after FooLive → MyApp.Foo.IndexLive and BarLive → MyApp.Bar.IndexLive — the brace form would collapse to alias MyApp.{IndexLive, IndexLive} instead of expanding to two separate alias lines. Personally, I'm not a fan of the brace alias form — it's always been a source of friction. That said, I spent another hour specifically hunting for edge cases in the brace handling and found the one above, which is now covered. I hope this will save another developer some headache. |
|
🚀 Thank you for your contribution! 🚀 |
|
I have to thank you so much for this wonderful project and the super quick responses! |
Closes #373
A few notes to make review easier:
Code style
mix formathas been run. Tests follow the same naming pattern as the existingrename_functionsuite (lib/example.ex,lib/some_module.ex, etc.). The new tests are grouped in adescribe "rename_module"block — that was a deliberate choice to keep them visually separated, but happy to flatten them into the existing list if you prefer consistency.Why the longer
@docthanrename_functionI spent a while working through the edge cases and the doc reflects that. Everything static is handled correctly: plain
alias, multi-alias (alias Foo.{Bar, Other}),alias Foo.Bar, as: B, submodules, test modules, string literals, and file moves. The longer doc makes the alias behaviour explicit upfront so callers know exactly what to expect without reading the source.Test modules are inferred and renamed together
FooTestis inferred from the old module name and renamed alongside it as a convenience — no configuration needed. Happy to make this opt-out via an option if you'd prefer.Quick walkthrough through the code
rename_modulefirst breaks the module down into all the forms it needs — string parts, atom lists for the AST, short form, test module variant. The logic then flows as a pipe:rewrite_affected_files— walks all files that mention the module and runs two passes per file: first AST (defmodule,alias,use, call sites, short forms viaexpand_alias), then a string replace for literals and commentsmove_submodule_files— moves any files whose path sits under the old module directory (e.g.lib/example/worker.ex→lib/new_example/worker.ex)move_module_file— moves the module's own fileContributor checklist
Leave anything that you believe does not apply unchecked.