Comments (7)
@philipgiuliani @mgwidmann Sorry, I forgot to ping you. This is part of v0.4.0-beta1
.
from credo.
I should really improve the explain
text for that check. 😕
The reasoning is that pattern matching in a condition will make the code harder to reason about, especially when maintained by a different person than the original author. Assigning in a condition can also be mistaken for a comparison (e.g. when working under deadline pressure).
Example:
if {:ok, %{"error" => true, "message" => message}} = parameter1 do
do_error_handling
end
In more complicated cases like this, the code might be more maintainable in the future if it reads like this:
case parameter1 do
{:ok, %{"error" => true, "message" => message}} -> do_error_handling
_ -> nil
end
I am guilty of writing code like this in the past, as I thought it to be clever and less verbose than the alternative. But the path to hell is paved with clever solutions and good intentions.
That said, nothing Credo suggests is set in stone. Maybe we should have a configuration option to allow simple assignments like the one in your example?
from credo.
I believe your example is no longer a conditional. Meaning, if the match fails it won't execute the code either way, thats certainly worth warning about.
However, in the most common use case of the match operator is actually a presence check.
if parameter = get_parameter_data() do
do_something_with(parameter)
end
Which is just shorthand for
parameter = get_parameter_data()
if parameter do
do_something_with(parameter)
end
In my experience, this is ok, as long as the match is entirely a wildcard (only a variable). I believe this error message should only trigger for "complex" matches, basically anything except a variable inside a conditional.
from credo.
Wanted to point out this is done in the Elixir codebase: https://github.com/elixir-lang/elixir/blob/574c6db3f8566a941b55c66499078d61661d08c6/lib/elixir/lib/kernel/cli.ex#L437-L448 and I'm sure in other places as well.
I would vote for a warning about any "complex" (or non-wildcard) type matches. Simply capturing the return value shouldn't be a problem unless it's the same name as another local variable/parameter (since that would then be bad if it changed to a ==)
from credo.
I heard you 😉 The behaviour will change like we discussed above.
Development is a bit slow at the moment, because there is a lot of stuff happening at my day job AND I am giving a talk about Credo at next week's ElixirConf.EU in Berlin and the preparation is taking up most of my free time. 😁
from credo.
Awesome! Just looked for the same thing. Its possible to use 0.4.0 already now?
PS: Your talk was amazing @rrrene ! That's why im using Credo now... :)
from credo.
I will release a beta this evening. Still working on the "create your own checks" UI, but most of the other stuff is done.
Will ping you here when it is released! 👍
from credo.
Related Issues (20)
- Predicate callbacks from behavior are not ignored HOT 2
- Issue with Unused variables confused about the strategy HOT 1
- missing spec on unquoted function raise error HOT 3
- Credo.Check.Warning.WrongTestFileExtension displayed in non-test module in v1.7.4 HOT 8
- NoAmbiguousAliases fails on function param with name alias HOT 3
- "Most of the time you are using the multi-alias/require/import/use syntax..." with compile_env HOT 4
- Gracefully handle output of bitstring `:message` in `%Credo.Issue{}` HOT 8
- False positive: spec in quoted HOT 3
- False Positive: Macro sigils with uppercase words HOT 4
- `UnusedKeywordOperation` check should ignore `Keyword.validate!/2` HOT 4
- Arguable advice from `Credo.Check.Refactor.AppendSingleItem` HOT 1
- Predicate function names should not start with 'is', and should end in a question mark flagged for :ets.whereis HOT 2
- Not ready for Elixir 1.16: a lot of 'warning: negative steps are not supported in Enum.slice/2 HOT 1
- Elixir 1.17 issues HOT 7
- Dependency divergence on compile HOT 2
- Unicode characters (arrows) that indicate priority of found issues not printed correctly HOT 2
- Credo.Check.Warning.Dbg: doesn't warn when using `&dbg/1`
- Error `mix credo` on Elixir 1.17 and Erlang/OTP 27 HOT 4
- Credo.Check.Design.SkipTestWithoutComment and many others, fail to run (Elixir 1.17 issue?) HOT 1
- Error upgrading project to Elixir 1.17 HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from credo.