Skip to content

7.1 Short Closures need better indentation #61

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

Closed
Nex-Otaku opened this issue Feb 14, 2023 · 5 comments
Closed

7.1 Short Closures need better indentation #61

Nex-Otaku opened this issue Feb 14, 2023 · 5 comments

Comments

@Nex-Otaku
Copy link

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
Copy link
Contributor

KorvinSzanto commented Feb 21, 2023

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
Copy link
Author

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
Copy link
Contributor

theodorejb commented Feb 26, 2023

@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
Copy link
Author

Nex-Otaku commented Mar 2, 2023

@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
Copy link
Contributor

KorvinSzanto commented Mar 8, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants