Allowing to ignore modules on Credo.Check.Readability.AliasAs #987
Conversation
Introducing a new `:ignore` param on `Credo.Check.Readability.AliasAs`,
to allow users to configure a list of modules that should be ignored by
this check. For example, with `{Credo.Check.Readability.AliasAs,
ignore: [Test]}` on the config, the check would not warn for `alias
Test, as: T` anywhere.
rrrene
left a comment
rrrene
left a comment
There was a problem hiding this comment.
Thx for putting this together! Please find my comments attached 👍
| do: issue_for(issue_meta, meta[:line], inspect(:__MODULE__)) | ||
| defp issue({:alias, _, [{:__MODULE__, _, nil}, [as: {_, meta, _}]]}, issue_meta, ignore) do | ||
| line = meta[:line] | ||
| {Credo.IssueMeta, source_file, _check_params} = issue_meta |
There was a problem hiding this comment.
Please use the functions in Credo.IssueMeta to deal with IssueMeta.
| defp issue({:alias, _, [{:__MODULE__, _, nil}, [as: {_, meta, _}]]}, issue_meta, ignore) do | ||
| line = meta[:line] | ||
| {Credo.IssueMeta, source_file, _check_params} = issue_meta | ||
| {_def, module_name} = Check.scope_for(source_file, line: line) |
There was a problem hiding this comment.
It seems expensive to get the scope for each occurrence instead of passing the module name during traversal.
| line = meta[:line] | ||
| {Credo.IssueMeta, source_file, _check_params} = issue_meta | ||
| {_def, module_name} = Check.scope_for(source_file, line: line) | ||
| module = Module.concat([module_name]) |
There was a problem hiding this comment.
We should use Credo.Code.Name.full/1 instead of Module.concat/1.
| {_def, module_name} = Check.scope_for(source_file, line: line) | ||
| module = Module.concat([module_name]) | ||
|
|
||
| if :__MODULE__ not in ignore and module not in ignore do |
There was a problem hiding this comment.
If :__MODULE__ is in ignore than we could have saved a lot of time calculating module.
| defp issue({:alias, _, [{_, _, original}, [as: {_, meta, _}]]}, issue_meta), | ||
| do: issue_for(issue_meta, meta[:line], inspect(Module.concat(original))) | ||
| defp issue({:alias, _, [{_, _, original}, [as: {_, meta, _}]]}, issue_meta, ignore) do | ||
| module = Module.concat(original) |
There was a problem hiding this comment.
We should use Credo.Code.Name.full/1 instead of Module.concat/1.
|
Thanks for the thorough review @rrrene. Will work on your suggestions soon! |
|
Checking in: Any developments? |
|
Checking in: Any news? |
|
Sorry for the delay. I finally am getting back on tracks after a huge hiatus in contributions. I'm focusing on this first, so I'll get back to you soon. |
2 participants
Closes rrrene/credo-proposals#81
This PR is introducing a new
:ignoreparam onCredo.Check.Readability.AliasAs,to allow users to configure a list of modules that should be ignored by
this check.
For example: with
{Credo.Check.Readability.AliasAs, ignore: [Test]}on the config, the check would not warn foralias Test, as: Tanywhere.