Block pipe check

[starbelly]

Closes rrrene/credo-proposals#37

There is still a good bit of TODO, in particularly around documentation and more specifically terminology, which I felt would be best served by discussion in a draft.

[starbelly]

Expression is probably too generic here, suggestions?

[Adzz]

fwiw I don't agree with this at all. case and if are just functions, so piping into them follows the convention in the rest of the language. And piping into a case statement is not an indication that your function is to big as we can trivially see with this example:

thing(stuff)
|> case do
   {:ok, stuff} -> stuff
   {:error, error} -> error
end

[starbelly]

A few points:

1. You definitely don't have to agree with it. I'd be willing to bet you don't agree with everything in credo as is, I know I don't. These things are all highly subjective. This one has been marked as controversial and not enabled by default to boot. It's essential that we all agree to disagree.

2. As to whether we consider case, if, or unless functions... I do not. At best and IMO these are macros. It all decompiles to erlang and in erlang these are keywords and definitely not macros or functions, but even this is arguable depending on the level of abstraction the developer chooses to operate at. However and IMO Elixir is Erlang. Once again, it's essential that we all agree to disagree.

3. When I say "big" I am definitely not referring to SLOC. I'm referring to how much knowledge a function has and further it can be said that every time a conditional expression is introduced in a function it's a branching point and may be (not definitely) time to create a new function that contains said conditional. The wording here definitely needs improving and it's all open to suggestion. But yet again, it's essential that we all agree to disagree.

4. I happen and others happen to think this is just not good style, this is really what it comes down to and you can provide no technical rationale for this, not unlike many other credo checks. I find it hard to read and as stated in the PR it IMO increases cognitive overload.

[rrrene]

I am in favor of adding this as a controversial check (just like you proposed). Can we brainstorm on the name once more? ExprPipe seems to tense (imho).

[starbelly]

@rrrene Yeah, I definitely don't like expr here, was too vague, possibly misnomer... what about Conditional ?

[rrrene]

Maybe this is more about piping into a function/macro that has a block, rather than about "conditionals" or "expressions".

Opinions?

[starbelly]

@rrrene Yeah, so this is much more specific I believe. That is to say, the code didn't try to catch other macros, just a very specific set, that being conditionals (if, unless, case). I could see it being adjusted to prevent piping to a bunch of Kernel functions/macros, but that was never my end goal.

[rrrene]

What I'm trying to say is: From a user's perspective, once you want to use some thing that "prevents" this

stuff
|> thing()
|> case do
   {:ok, stuff} -> stuff
   {:error, error} -> error
end

wouldn't you also want to prevent this?

stuff
|> thing()
|> my_macro do
   # ...
end

because the feeling/reasoning that triggered the creation of this check was not about any specific macro (if, with, case) but rather about piping "into" a block.

I'm not sure, but I think this could be the real benefit of a check like this.

[starbelly]

@rrrene in that sense, yes I fully agree, as I would prefer not to pipe to a try block either. So, perhaps there is more work to be done on in this check, specifically catching anything that is keyword do, and the check could be renamed to PipeBlockCheck. Thoughts?

[rrrene]

@starbelly Sounds perfect 👍

[starbelly]

@rrrene 👍 Changed the code, module name, added a test for try do, need to update the docs a bit... coming 🔜

[starbelly]

@rrrene Docs updated, really trimmed them, as it's folly to try and provide a technical rationale here. Unless you or someone has suggestions IMO this is good to go 👌

[OvermindDL1]

A receive block would be another.

But yeah, this should definitely not be enabled by default. I pass in to case substantially for such ok/error destructuring.

[starbelly]

@OvermindDL1 Indeed.

[rrrene]

@starbelly Thx, great work! 👍 I added an :exclude param so that users like @OvermindDL1 can configure their personal exceptions like this

{Credo.Check.Readability.BlockPipe, exclude: [:case]}

[starbelly]

@rrrene Thanks for adding that, oversight on my end.