php-fig / per-coding-style

PER coding style
262 stars 26 forks source link

7.1 Short Closures need better indentation #61

Closed Nex-Otaku closed 1 year ago

Nex-Otaku commented 1 year ago

Look at the indentation in short closure multiline example.

The indentation bounces wildly, it feels not right.

It's because blocks go in form that is not consistent with all other constructs.

image

Better:


$func = fn(
        int $x,
        int $y,
    ): int
    => $x + $y;
KorvinSzanto commented 1 year ago

The declaration is consistent with other multiline declarations as-is. For example:

# Anonymous classes
// Brace on the next line
// Constructor arguments
$instance = new class($a) extends \Foo implements
    \ArrayAccess,
    \Countable,
    \Serializable
{
    public function __construct(public int $a)
    {
    }
    // Class content
};

# Anonymous function declaration
$noArgs_longVars = function () use (
    $longVar1,
    $longerVar2,
    $muchLongerVar3,
) {
   // body
};

# Multiline function call (Note the closing `)` has the same scope indentation as the scope it's declared in
$foo->bar(
    $arg1,
    function ($arg2) use ($var1) {
        // body
    },
    $arg3,
);

In general if you need a multiline short closure you should probably be doing something else anyway.

Nex-Otaku commented 1 year ago

No.

image

Compare it to case of short closure.

You will see a difference.

All that examples that you provide look just fine.

Short closure one does not.

theodorejb commented 1 year ago

@Nex-Otaku Your suggested change also doesn't start and finish on the same indentation level. It just changes the indentation of the closure parameters, making them inconsistent with all other function parameters.

Nex-Otaku commented 1 year ago

@theodorejb you are right, my suggested change also does not start and finish on the same indentation level.

However my goal was to state the problem as much clear as I can.

That's what issues are for, right? To bring some important enough topics for a discussion. It's not a Pull Request.

Not to provide the best possible solution - because I dont have one. Do you?

If anyone will made better suggestion I would be happy to give my voice for it.

I don't care much who will fix that and how.

Maybe we can just remove short closure multiline example as a whole if there will be no good enough solution.

It's not me to decide.

Just think about it. Do you like it as it is now?

KorvinSzanto commented 1 year ago

I think the reality here is that breaking up one of these short functions across multiple lines is basically always a bad idea, the best we can do is try to give consistent guidelines for when it might happen and hope that folks just avoid splitting short functions across multiple lines all together. For my own code I'll be using a full anonymous function rather than a short function any time I need more than one line.

Thanks for the thorough write-up in this issue, I really appreciated the images as well!