Add new AliasOrder check #547
Conversation
remi
force-pushed
the
feature/alias-order-check
branch
5 times, most recently
from
f2ed576
to
35da983
Compare
|
@remiprev 馃憤 that looks pretty good already! One thing I overlooked in the tests is supporting multi-alias syntax like alias Credo.Priority
alias Credo.CLI.{Sorter,Filename,Options}Could you add those to the tests and ensure that Multi-Alias is ordered as well? |
remi
force-pushed
the
feature/alias-order-check
branch
3 times, most recently
from
0242438
to
098e5b6
Compare
|
@rrrene Done! 馃帀 I added two new tests and made them pass. |
remi
force-pushed
the
feature/alias-order-check
branch
2 times, most recently
from
f4c9104
to
7dcea78
Compare
| end | ||
|
|
||
| group | ||
| |> (&apply(Enum, chunk_method, [&1, 2, 1])).() |
There was a problem hiding this comment.
There is a module called Credo.Backports.Enum where you can add a chunk_every function and delegate it based on the user's Elixir version (hint: chunk_every was introduced in Elixir 1.5).
| format_issue( | ||
| issue_meta, | ||
| message: "The alias `#{trigger}` is not alphabetically ordered among its group.", | ||
| trigger: trigger, |
There was a problem hiding this comment.
The trigger has to be the substring which actually causes the issue in the given line.
Third party tools, like editor integrations, use this to highlight the portion of the source code that is causing the issue.
In your implementation, you expand all the multi-alias members to check their order, which is fine. But you can not use the expanded version for the trigger field.
There was a problem hiding this comment.
Thank you for your feedback, the code is now much cleaner! There鈥檚 only this comment left and I want to make sure I get the specs right before I start implementing it 馃槃
defmodule Text do
alias Module2.Module20
alias Module1.Module10
endThe trigger should be "Module1.Module10".
defmodule Text do
alias Module1.Module10
alias Module1.Module09.Module009
endThe trigger should be "Module09.Module009".
defmodule Text do
alias Module1.Module10
alias Module2.{Module20, Module19}
endThe trigger should be "Module19".
defmodule Text do
alias Module1.Module10.Module100
alias Module2.Module20.{Module202, Module201, Module203}
alias Module3.Module30.Module300
endThe trigger should be "Module201".
Does that make sense?
There was a problem hiding this comment.
@rrrene I modified my implementation to fix trigger. We now refrain from converting AST modules into strings except when we really need them.
I also fixed a bug where line_no was not computed properly when multi-alias expansion was used on multiple lines.
Let me know if you鈥檇 like me to fix something else!
| end | ||
|
|
||
| defp find_alias_groups( | ||
| {:alias, _, [{:__aliases__, [line: line, column: _], mod_list}]} = ast, |
There was a problem hiding this comment.
You can not match on [line: line, column: _] like this, because it is a keyword list and it will only match if both and only both values are present.
If, at some point in the future, we get another field in the metadata of the AST, our code would be incompatible between Elixir versions (this actually happened in the past when column: was introduced).
| ), | ||
| do: process_single_alias(ast, line, mod_list, aliases) | ||
|
|
||
| defp find_alias_groups({:alias, _, [{:__aliases__, [line: line], mod_list}]} = ast, aliases), |
| defp find_alias_groups( | ||
| {:alias, _, | ||
| [ | ||
| {{:., _, [{:__aliases__, [line: line, column: _], mod_list}, :{}]}, _, multi_mod_list} |
| defp find_alias_groups( | ||
| {:alias, _, | ||
| [ | ||
| {{:., _, [{:__aliases__, [line: line], mod_list}, :{}]}, _, multi_mod_list} |
| end | ||
|
|
||
| defp find_alias_groups( | ||
| {:alias, _, [{:__aliases__, [line: line, column: _], mod_list}]} = ast, |
There was a problem hiding this comment.
You can not match on [line: line, column: _] like this, because it is a keyword list and it will only match if both and only both values are present.
If, at some point in the future, we get another field in the metadata of the AST, our code would be incompatible between Elixir versions (this actually happened in the past when column: was introduced).
That's why we have do this by hand: Example from the AliasUsage Check
remi
force-pushed
the
feature/alias-order-check
branch
5 times, most recently
from
4c9c7ad
to
c46ed4d
Compare
remi
force-pushed
the
feature/alias-order-check
branch
from
c46ed4d
to
c53aed7
Compare
|
Thanks @remiprev 馃憤 |
|
My pleasure! Do you need me to create another pull request to add a line in |
|
@remiprev No need, I'm already on it ;-) |
There are already tests for this check, waiting to be implemented. They now all pass.
@rrrene This is my first time implementing a check as complex as this one, so I鈥檓 looking for feedback if I should have done things differently.
Thank you! 馃槉