Pipe chain should start with a raw value. about credo

[chrismcg]

Thanks for Credo from me too. Another example from my code is:

    Stream.resource(&stream_start/0, stream_data(api_call_fn), &stream_end/1)
    |> Stream.each(fn(data) -> send(caller, {message, data}) end)
    |> Stream.run

Where I have to introduce a variable to prevent the warning

    stream = Stream.resource(&stream_start/0, stream_data(api_call_fn), &stream_end/1)
    stream
    |> Stream.each(fn(data) -> send(caller, {message, data}) end)
    |> Stream.run

Is there any plan to introduce something like # credo:disable Refactor.PipeChain ?

[whatyouhide]

@hickscorp the fact that that looks better is very much a matter of opinion. Personally, I think it's extremely confusing because of all the indentation and all the things going on. I'd much rather have things named:

a =
  try do
    ...
  end

{reply, state} =
  case a do
    ...
  end

{:reply, reply, state}

[rrrene]

@chrismcg There is, although I am still undecided on the format, since I personally dislike "magic comments". I would like to see a more "Elixiry" solution and I might just have had an idea 👍

P.S. While I know this does solve the actual problem we are discussing, wouldn't this be the "Credo compatible solution"?

&stream_start/0
|> Stream.resource(stream_data(api_call_fn), &stream_end/1)
|> Stream.each(fn(data) -> send(caller, {message, data}) end)
|> Stream.run

[rrrene]

I don't disagree. Your examples show that we need exceptions from the rule.

The value of this check is when you do things like:

String.downcase(params[:user_name])
|> valid_username?()

I would argue that

params[:user_name]
|> String.downcase
|> valid_username?()

is so much clearer.

So what would you propose as "exceptional cases"? Ranges seem like a good candidate, like in your first example.

[seantanly]

I agree with your example, it can seem clearer. In fact, I'm habitually used to writing the pipe style as I code along, in anticipation that the output may be used in further transformation. Perhaps, I'm so used to looking at functions being called via pipe, that function calls with 1 less argument within the parentheses looks less cluttered/nicer to read.

E.g. I like

values = input_str |> String.split(~r/some_long_complex_ugly_looking_regex/, trim: true)

instead of

values = String.split(input_str, ~r/some_long_complex_ugly_looking_regex/, trim: true)

However, the convention inside Elixir source code suggests otherwise. The pipe operator seems avoided when the output isn't further piped into another function.

In short, what I'm saying is this might be an area which boils down to personal taste. And it might be generating more noise than signal to suggest this refactoring.

[rrrene]

There is not much difference between our positions, I think.

We both agree that the "thing" going into the pipe should be clear. I see that in your initial examples, so I think rather than removing the check, I would like to search for sane defaults like "blocks and ranges can go into a pipe" with a possibility for configuration.

Will try to incooperate something into the 0.3.0 release 👍

[chrismcg]

Great to hear you have plans. In ruby I have rubocop as part of CI and it fails the build if it isn't happy and so being able to disable it for a particular section of code is necessary sometimes. At the moment I've Credo turned off on CI for my Elixir projects because I can't tell it "I know, I'll fix it later but it's not the most important thing to be doing right now"

I hadn't thought of your approach above. I don't think it's something I'd do though.

foo
|> bar
|> baz

The code above tends to imply, in my mind at least, that foo here is being passed through each function, like conn in Phoenix or the stream in the example we've been talking about. I'm not passing the stream_start function through each call to stream so having it bare at the top of the pipe might be misleading compared to normal usage. Have to think about it more though.

[rrrene]

Yeah, I know what you mean with the conn example. I have had the same problem with actual production code that I thought to myself "is it really okay that the type changes constantly between pipes"? Is that maintainable code?

Contrived example:

input_str                             # string with usernames
|> String.split(",")                  # list of usernames
|> Enum.map(&find_user_by_name/1)     # list of %User{} 
|> Enum.filter(&(&1.email_enabled?))  # list of %User{} 
|> Enum.each(&send_reminder/1)        # :ok

I am confident that we, as a community, will work things like these out over time. There's more open source Elixir code every day and more production code as well. Good times ahad 😁

[chrismcg]

Agree there are good times ahead. I'm hoping later in the year I'll have more to contribute to Credo than examples on issues! In the meantime I still love chatting about this stuff though...

With the input_str example although you don't pass the string all the way through the rest of the pipe is concerned with transforming it. &stream_start/0 is only used once in the Stream.resource call so I don't think it fits the metaphor of pipes transforming an input to an output.

[rrrene]

You're right, "taking and transforming data" is the purpose of pipes and the &stream_start/0 example does not do that.

I will work on a method to exclude individual parts of code from certain checks 👍

[rrrene]

@chrismcg Credo v0.3.0 now includes support for a @lint attribute. See CHANGELOG for further details.

What do you think?

[chrismcg]

Looks great, thanks! Module attributes are a perfect solution for this.

[seantanly]

Thanks for @lint! And together with .credo.exs gives us full ability for customization.

[hickscorp]

I would really like this to be extended to case statements too... For example:

    try do
      f.(data)
    rescue
      e -> {:error, {:ex, e, System.stacktrace()}}
    end
    |> case do
      {:ok, new_data} -> {:ok, {name, new_data}}
      err -> {err, state}
    end
    |> Tuple.insert_at(0, :reply)

looks really better than the non-pipe version. Am I missing something?

[rrrene]

@hickscorp I just answered the same proposal of yours here. As @whatyouhide suggests, this is very much a matter of personal taste.

Also, I would be open to the possibility that this check is not "good" after all. Maybe it had the right intentions, but the current implementation is not the right way to accomplish them.

IMHO both refactorings Andrea and I suggested are what Credo should suggest. As I replied to the same proposal in the other issue, calling Tuple.insert_at/3 might make your function less comprehendable and unnecessarily confusing.

[hickscorp]

@rrrene As I stated in the other issue, I agree to your comment, that it's a matter of taste. Therefore, a way of configuring that behaviour globally would be great (Eg disallow impure pipping unless the target function is within a list of arrity, much like how mix format allows to exception-list the use of parentheses) using a configuration file.

@whatyouhide I agree that my example wasn't the best from a clarity perspective. There are other examples that are much clearer, with just a list of things being pipped through lots of list operations, and in the end to a case statement, for example.

What do you think?