rrrene / credo-proposals

Proposals for Credo, the Elixir code analysis tool with a focus on code consistency and teaching
MIT License
9 stars 0 forks source link

Proposal: Change to pipe chain should start with a raw value #41

Open cdeyoung opened 5 years ago

cdeyoung commented 5 years ago

Credo version: 1.0.0 Erlang: 21.2.4 Elixir: 1.8.0 macOS

Current behavior

Currently, Credo complains about code like this:

from( u in User, order_by: u.inserted_at ) |> Repo.all()

or

Path.join(__DIR__, "file.txt") |> File.read!()

with the message "Change to pipe chain should start with a raw value." While I usually agree that function chains should begin with raw values, the examples above seem like good cases for not starting with a raw value. I could store the from() function's result in a variable and then pass the variable into Repo.all(query), like this:

query = from(u in User, order_by: u.inserted_at) Repo.all(query)

Or pull out the first parameter, like this:

__DIR__ |> Path.join("file.txt") |> File.read!()

but these both feel unnecessary and difficult to read.

Expected outcome

It would be nice to be able to exclude function names from the Change to pipe chain should start with a raw value check for cases where it is more readable NOT to start a function chain with a raw value.

axelson commented 5 years ago

I'd actually like to vote for removing PipeChainStart from the list of default checks and make it opt-in only since there are many cases starting from a non-raw value is much more readable and stylistically I don't believe that can be determined programmatically by Credo.

markevich commented 5 years ago

Here is another bad example of "Change to pipe chain should start with a raw value." rule.

(a * b) |> Float.round(4)

is more readable(in my opinion) than

a |> Kernel.*(b) |> Float.round(4)
Hajto commented 5 years ago

The other problem with that is when you have test factory functions that just give you some example data based on file:

def test_data, do: File.read!(@path_to_fixture)

credo will keep complaining about:

assert {:ok, result} = test_data()
              |> do_some_work
              |> do_more_work
pggalaviz commented 5 years ago

I also vote for removing PipeChainStart from the list of default checks.