-
Notifications
You must be signed in to change notification settings - Fork 73
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
bump deps #69
bump deps #69
Conversation
a226b0b
to
dd9f373
Compare
@@ -7,6 +7,7 @@ | |||
excluded: [] | |||
}, | |||
checks: [ | |||
{Credo.Check.Readability.ParenthesesOnZeroArityDefs, [parens: true]}, |
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.
Why do we need this? It's a common approach to skip this
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.
Credo enforces either we use ParenthesesOnZeroArity or we don't. Since using them required less changes to the codebase, I went and set parens to true.
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 I comment this out I get
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:5 #(Ethereumex.Config.setup_children)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/counter.ex:8 #(Ethereumex.Counter.setup)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:87 #(Ethereumex.Config.ipc_request_timeout)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:82 #(Ethereumex.Config.ipc_max_worker_overflow)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:77 #(Ethereumex.Config.ipc_worker_size)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:62 #(Ethereumex.Config.ipc_path)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:57 #(Ethereumex.Config.client_type)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:52 #(Ethereumex.Config.http_options)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:37 #(Ethereumex.Config.rpc_url)
┃ [R] ↘ Do not use parentheses when defining a function which has no arguments.
┃ lib/ethereumex/config.ex:27 #(Ethereumex.Config.poolboy_config)
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.
Why do we need parentheses?
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.
-
It was a matter of choice between fixing 5 issues or 10 :) haha
-
I personally prefer parentheses because functions look weird without it and no such thing exists in erlang. And to keep the consistent look between functions. :)
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.
It's more conventional to skip parentheses so let's revet these changes
https://github.com/phoenixframework/phoenix/blob/master/lib/phoenix.ex
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.
Depends what you're reading:
https://github.com/lexmag/elixir-style-guide/blob/master/README.md#fun-parens
devs from @elixir-lang core team use that.
Elixir Logger uses it:
https://github.com/elixir-lang/elixir/blob/master/lib/logger/lib/logger.ex#L580
Elixir ExUnit doesn't use it:
https://github.com/elixir-lang/elixir/blob/master/lib/ex_unit/lib/ex_unit.ex#L297-L306
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.
Ok. Please bump version, add a changelog and let's merge this
mix.exs
Outdated
{:jason, "~> 1.2"}, | ||
{:credo, "~> 1.3", only: [:dev, :test], runtime: false}, | ||
{:ex_doc, "~> 0.21", only: :dev, runtime: false}, | ||
{:dialyxir, "~> 1.0.0", only: [:dev, :test], runtime: false}, |
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.
1.0?
@InoMurko squash commits please and we can merge it |
No description provided.