-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Move explain to standalone task. #189
Conversation
end | ||
|
||
defp explanation_text(warning_name) do | ||
warning = String.to_atom(warning_name) |
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.
I'm always a bit weary of String.to_atom/1
. Should be fine here, though, I think.
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.
Yeah its the best choice here, we can't leak.
lib/mix/tasks/dialyzer/explain.ex
Outdated
defp explanation_text(warning_name) do | ||
warning = String.to_atom(warning_name) | ||
module = Map.get(Dialyxir.Warnings.warnings(), warning) | ||
(module && module.explain()) || "Uknown warning named: #{warning_name}" |
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.
explain/1
will always return a truthy value, so this check is really
if module do
module.explain()
else
"Unknown warning named: #{warning_name}"
end
Also typo on Unknown
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.
Or if you REALLY want to get fancy:
case Map.get(Dialyxir.Warnings.warnings(), warning) do
nil ->
"Unknown warning named: #{warning_name}"
module ->
module.explain()
end
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.
If the warning name isn't found in the map, module
will be nil so the left branch will fail and it prints the unknown. Do you just mean the if
/case
is more readable?
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.
Yeah I think it's a little more honest as to intent. They're all functionally the same.
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.
After comments and tests pass 😄
68699c7
to
48889d5
Compare
Might be worth mentioning the new task in the README too |
5bd059b
to
67675ed
Compare
resolves #173