Plugin-php: Option for opening brace

[mgrip]

1st off I want to be clear - I think the default should still be to follow PSR2-like formatting standards

With that in mind, from my own experience and now starting to look through other open-source php projects, it seems like one of the biggest differences in formatting standards is putting the opening brace for functions, classes, etc on a new line vs same line. I'm a little worried that not having this option might scare people off from using prettier/plugin-php.

I was thinking of starting to work on a PR for adding an option to do this (defaulting to the PSR2 format of putting it on a new line), but wanted to open for discussion first.

openingBraceNewLine = true (default)

class Test
{
  public function testMethod()
  {
  }
}

function testFunction()
{
}

openingBraceNewLine = false


class Test {
  public function testMethod() {
  }
}

function testFunction() {
}

[mgrip]

cc @evilebottnawi @czosel

[czosel]

For me, that line break definitely is the ugliest aspect of PSR-2, so from a user perspective i'd support this 100%. From a maintainance perspective, I think it depends on the implementation complexity - if this boils down to a couple of ternarys printing hardlines or not (which it probably will), then I'd say let's go for it!
I guess we should leave the default to be PSR2-compatible, right?

[mgrip]

Totally agree. I'm taking a look now and I think it should definitely be doable - I think it just boils down to classes, functions, methods, and traits - which all extend the statement type which should help. And yes definitely agree we should leave the default to be PSR2-compatible.

Ok cool I'll try and put together a PR and we can see how much complexity it adds

[mgrip]

Added in #108

[mgrip]

Reopening for more discussion - @evilebottnawi

[alexander-akait]

@mgrip thanks, be good if we revert option and keep PR open

[mgrip]

I'll put up an MR to remove it - it's just removing 1 loc of code from the printer. How do you propose we get more insight/opinions?

[alexander-akait]

@mgrip twitter + issue

• :+1: Add
• :-1: Don't add

We have other many problem right now, new option should be added only after stable release

[mgrip]

@evilebottnawi @czosel I still want to continue this discussion - Like I mentioned in the original issue posting I think this could scare people off from using prettier for php. Several large open-source php projects (Wordpress, Drupal, Facebook's Hack) do not put the opening brace on a new line (while plenty of others following PSR2 do)

[alexander-akait]

@mgrip we should ping here WordPress, Drupal and other main constributors

[mikecrittenden]

Drupal dev checking in. Not supporting this means that no Drupal devs are going to touch it.

[alexander-akait]

@mikecrittenden new options will be only after stable release

[antonioribeiro]

You prefer K&R and some other like Allman better, like I do.

In my opinion Allman is beautiful, clearer and better, only because it brings symmetry to the table: a brace/bracket should end in the exact same position the opening one started. Yes, you get some extra lines with it, but you're not supposed to bloat your methods anyway, right? So, I don't think it's a problem.

So, if we're talking about going K&R where we now have Allman's, I think we should also do the opposite, for conditionals and loops, don't you think?

[virgofx]

Allman is NOT PSR-2 nor matches any of the other defaults in prettier from the JS side. Everyone has an opinion that differs (good for you). At some point an option may be added, otherwise, don't use it. Prettier is designed to be opinionated -- and it's worked incredibly well in the JavaScript side ... I'm guessing the same thing will happen very shortly once stable is released in PHPland.

I agree an option should exist ... but defaults out of box should be close to PSR-2/JS-prettier as possible (similar to @mgrip 's initial assessment) So default would not be Allman.

[antonioribeiro]

PSR-2 is using Allman for classes and methods:

class Test
{
  public function testMethod()
  {
  }
}

And K&R for conditionals and loops:

if ($a === $b) {
    bar();
} elseif ($a > $b) {
    $foo->bar($arg1);
} else {
    BazClass::bar($arg2, $arg3);
}

They don't talk about Allman in the docs, but they are not consistent on their design too.

[virgofx]

You're right they're inconsistent. I would recommend follow the current PSR-2 which is Allman for classes/method + K&R for conditions/loops.

[roippi]

Hi, can we maybe revisit our decision to couple this change to a post-1.0 release?

1.0 is dragging on a bit - which is fine - but this is the primary blocker for using prettier at my company.
Like the drupal folks, there are many here who "won't touch" prettier without this option. I'd really rather not have to fork in order to use.

[alexander-akait]

@roippi can you describe what you have what you see now and what you expected with examples

[czosel]

@evilebottnawi I suppose what many people would like to see is K&R style for classes and methods:

class Test {
  public function testMethod() {
  }
}

[roippi]

Hi @evilebottnawi, we have (and would like to keep) opening brace on same line for classes and methods, per @czosel example. Thanks.

[mgrip]

I would agree with @roippi - seems like it still might be a while until we release a 1.0, and the PR to add this option would be pretty straightforward

[antonioribeiro]

There's no way to please everyone without an option here.

--braces <psr2|allman|kr>

[roippi]

Would a PR to add --braces be welcome at this point?

[alexander-akait]

@roippi no, only after stable release, also we need discussion about this

[aboyton]

@roippi no, only after stable release, also we need discussion about this

What more discussion are you after? I feel that this topic has been discussed to death both in this issue and https://github.com/prettier/plugin-php/issues/120

The only option people are asking for is https://github.com/prettier/plugin-php/issues/120#issuecomment-386606483, that is presently we have:

1. Opening brace for classes are on a new line
2. Opening brace for functions are on a new line
3. Opening brace for other blocks are on the same line
4. Braces are auto-inserted for one-line statements
   I (and others) are all asking for an option to change 1 and 2 to be on the same line, following the conventions that Phabricator (and thus I assume Facebook) and Wordpress follow.

Would you like more discussion about which options people want or discussion about this particular option (namely a single option to change 1 and 2 to be on the same line).

[alexander-akait]

@aboyton Some people ask here --braces <psr2|allman|kr>, this violates https://prettier.io/docs/en/option-philosophy.html .

What is the name of the option you offer? And what it should change (please provide examples)? This will allow us to think over the option(s) in more detail.

[aboyton]

I linked to what I was after in the previous comment, but let me place it here on this issue. I'd love to have the following code style:

<?php
class test {
    public function another_test() {
        if (true) {
            print 'hi';
        }
    }
}

The name of this option seems to be most commonly referred to as 1TBS. Please read through the discussion on https://github.com/prettier/plugin-php/issues/120 rather than forcing a repeat of all of the discussion again on this issue. Many of the core contributors commented their positions.

[czosel]

So probably we'd call the option brace-style with the options

• psr2 (default)
• 1tbs
  Correct?

[alexander-akait]

@manspaniel option is not disabled, it is not implemented right now, we want release stable version first, anyway PR welcome

[alexander-akait]

/cc @aboyton we can implement this in near future, but there a lot of difference requests, need clarify.

Places:

1. Classes (newline now)
2. Function (newline now)
3. Control structures (same line now)
4. Anonymous classes (same line now)

What we can do:

1. Implement --classes-bracket-same-line (default: false) affects only on classes/interfaces/traits.
2. Implement --function-bracket-same-line (default: false) affects only on functions/method.
3. Don't think we really need this
4. Don't think we really need this

[czosel]

I don’t see why one would have different settings for classes and functions. I’d still propose what was mentioned above:

brace-style

• psr2 (default, as now)
• 1tbs (no line break for classes and functions)

[aboyton]

I'm with @czosel. We don't need functions and classes to be different to each other, we'd want two options either psr2 or 1tbs.

[antonioribeiro]

@evilebottnawi ,

1. Don't think we really need this
2. Don't think we really need this

I think we do. At least for control structures. Newline (Alman's coding style) is the original Laravel coding style (before it moving to PSR-2), but which is still being used, so, a while ago, I wrote this PHP_CS plugin to support it: https://github.com/antonioribeiro/laravelcs.

Code in Allman's is symmetric, and, thus, Prettier:
https://github.com/laravel/framework/blob/v4.2.11/src/Illuminate/Database/Eloquent/Builder.php#L208
https://github.com/laravel/framework/blob/v4.2.11/src/Illuminate/Database/Eloquent/Builder.php#L184
https://github.com/laravel/framework/blob/v4.2.11/src/Illuminate/Database/Eloquent/Builder.php#L701

Please don't let it out of Prettier PHP.

[alexander-akait]

@antonioribeiro can you provide example for 3 and 4?

@aboyton @czosel what about closure?

I am afraid what 1tbs is not enough for some developers

[czosel]

I'm actually quite convinced that the minimal set of options (1tbs / psr2) is the only way to stay compatible with prettier's philosophy: We should only introduce options when not doing so is a _major_ roadblock for adoption. This is the case for 1tbs/psr2, because both styles are very popular in the community. I think the fact that laravel moved to PSR-2 actually confirms that Alman style on control structures should not be considered anymore.

[alexander-akait]

@czosel So we implement 1tbs (space instead newline for functions/classes), right?

[czosel]

@evilebottnawi Yep, that's what I'd strongly suggest. We can still discuss adding more options later, but that should be done in a separate issue.

[alexander-akait]

WIP, will be release as 0.10.0 release

[antonioribeiro]

@evilebottnawi, aren't those links I sent examples for control structures and closures?

[alexander-akait]

@antonioribeiro yep, just made sure I understood correctly

[alexander-akait]

@czosel What better name for option? --opening-brace or --opening-brace-style? Or maybe you have better idea for name?

[czosel]

I’d just call it „brace-style“, plain and simple 🙂

[antonioribeiro]

Depends on the options we will have, but the simpler the best: --brace-style, as said by @czosel, is great.

[alexander-akait]

What about this:

class ClassName extends ParentClass implements
    VeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongFileName1
{
}

Should we use space here too? For me better use newline here for readable.

[mgrip]

👋 Might want to just mimic JS to start (keep same-line)

[alexander-akait]

Review welcome https://github.com/prettier/plugin-php/pull/918

[Taro-Naza]

You prefer K&R and some other like Allman better, like I do.

In my opinion Allman is beautiful, clearer and better, only because it brings symmetry to the table: a brace/bracket should end in the exact same position the opening one started. Yes, you get some extra lines with it, but you're not supposed to bloat your methods anyway, right? So, I don't think it's a problem.

So, if we're talking about going K&R where we now have Allman's, I think we should also do the opposite, for conditionals and loops, don't you think?

I definitely like Allan better it makes the code cleaner and easier to read and follow up.

[Taro-Naza]

I agree I think it should be an option for all languages. I use it for Javascript and I would love to have the freedom to choose what to use.

[alexander-akait]

Please read https://prettier.io/docs/en/option-philosophy.html

[antonioribeiro]

Being opinionated means you have very strong opinions. But... Where are those opinions coming from? How do you build them? And, above all, are them changeable or set in stone? If so, how do we change them?

I believe "Prettier", means it should be, first of all, Pretty. By not choosing Allman is good example of PSR2 choosing the uglier one. When the PHP community was trying to force the Laravel community (and they won), we had a beautiful coding style, Taylor Otwell was proud of using mostly Allman in Laravel core, and you should be able to see in this thread most of us in favor for keeping Allman instead of moving to PSR2, because Allman is symmetric, and pretty.

[alexander-akait]

@antonioribeiro we are always open for improvements, no need to quote that when someone said something, it’s better to show examples of why this is better and in what cases, we are considering the possibility of changing some things, so the more information there is, the better and more convenient we will design

[antonioribeiro]

@evilebottnawi, beauty is a matter of taste, and sometimes, even knowing it's ugly, people feel beauty is the least important matter on some subjects. But, not having something really consistent to say about it, people said PHP was the uglier language around:

So my questions are nothing but honest, because I really think that if a community is building an opinionated software, how are the opinions forged?

As for examples, I can't provide many, but here's some code from Symfony formatted according to PSR2:

if (SortableIterator::SORT_BY_ACCESSED_TIME === $mode
    || SortableIterator::SORT_BY_CHANGED_TIME === $mode
    || SortableIterator::SORT_BY_MODIFIED_TIME === $mode) {
    if ('\\' === \DIRECTORY_SEPARATOR && SortableIterator::SORT_BY_MODIFIED_TIME !== $mode) {
        $this->markTestSkipped('Sorting by atime or ctime is not supported on Windows');
    }
    $this->assertOrderedIteratorForGroups($expected, $iterator);
} else {
    $this->assertOrderedIterator($expected, $iterator);
}

Let's make it Prettier using Allman's and one extra return (absorbed by me from the Laravel's coding style):

if (SortableIterator::SORT_BY_ACCESSED_TIME === $mode
    || SortableIterator::SORT_BY_CHANGED_TIME === $mode
    || SortableIterator::SORT_BY_MODIFIED_TIME === $mode) 
{
    if ('\\' === \DIRECTORY_SEPARATOR 
        && SortableIterator::SORT_BY_MODIFIED_TIME !== $mode) 
    {
        $this->markTestSkipped(
            'Sorting by atime or ctime is not supported on Windows'
        );
    }

    $this->assertOrderedIteratorForGroups($expected, $iterator);
} else {
    $this->assertOrderedIterator($expected, $iterator);
}

[alexander-akait]

@antonioribeiro lets open new issue i want to know how many people need this

[fabd]

I also like the opening brace on its own line, I find it makes the code more readable for functions and classes. It does make the code seem more verbose in conditionals, and creates unnecessary vertical space that feels inconsistent.

Hence I think the middle way would be the best. An option to allow opening braces on their own line, for functions and classes. For conditionals, the code looks more tight and less verbose with braces on the same line.

I think this would be perfect for both php and Javascript formatting.

[mean-cj]

+1 for brace-style options ( allman )

[mean-cj]

Hi ,
if you need if,else,elseif for brace style to Allman
you can modify
C:\Users\USERNAME.vscode\extensions\esbenp.prettier-vscode-5.9.2\node_modules\@prettier\plugin-php\src\printer.js

Change Line: 1863

     if (node.alternate) {
        parts.push(node.shortForm ? "" : "} ");

to

      if (node.alternate) {
        parts.push(node.shortForm ? "" : "}", isFirstChildrenInlineNode(path) || !node.body ? "" : hardline );

input

if (1) {
} elseif (1) {
} else {
}

output

if (1) {
}
elseif (1) {
}
else {
}

[fabd]

Ah yes I'd say I'd like a fix for that eveb more.

Forcing a closing and opening brace on the same line, misaligns the 'else' statements. It's just not as readable.

It's inconvenient as well when you refactor and you just want to move or delete one of those else/elseif statements. With eg. Vim you'd just delete a block of lines, and then pop it elsewhere. No hanging characters to worry about. If you move such a conditional higher/lower in the same containing block, it doesn't even need reformatting.