Comment 1 Natalia Bartol 2013-05-24 10:20:11 EDT

Created attachment 231471 [details]
Incorrect array formatting - unit test

Comment 2 itay friedman 2013-06-03 10:41:48 EDT

Created attachment 231877 [details]
patch for reverting the changes leading to this regression

the patch reverts the changes, solving this issue, while 406357 also remains resolved

Comment 3 Natalia Bartol 2013-06-04 05:10:35 EDT

Patch committed. Changes introduced by 406357 have bee reverted. The original bug reported in 406357 is fixed.
Unit tests:
* 4 new unit tests added to org.eclispe.php.ui.tests.formatter-autoedit
* 2 new tests added to org.eclipse.php.formatter.core.tests

Thanks Itay!

http://git.eclipse.org/c/pdt/org.eclipse.pdt.git/commit/?id=f7831d158be50313aac9af1fb58e51a8f9c0ecf2

Comment 4 Jacek Pospychala 2013-06-05 13:35:27 EDT

Natalia,
it seems you reverted too much. Robert's patch included extension point indentationStrategy, which was now just removed with Itay's patch.

Was this extension point problematic? If not, could you restore it?

Comment 5 Natalia Bartol 2013-06-05 14:09:38 EDT

I thought this extension point was only for fixing the original bug.

Sure, we can add it back but please open a separate bug and submit a patch that adds this new extension point + example Java code with dummy contribution + proper documentation in exsd file (see Bug 408004 as a reference).

Thanks

Comment 6 Jacek Pospychala 2013-06-05 16:00:05 EDT

Well work out missing ext.point docs in coming days but for now I need correct PDT build for testing signing so I reverted Itay's patch to restore lost extension point.
I'll be back to this issue tomorrow and will try to reapply his patch without removing extpoint and associated logic.

Comment 7 Natalia Bartol 2013-06-05 16:18:25 EDT

Why build without this extension point is incorrect? All tests pass.

Comment 8 exceptione 2013-06-12 08:09:47 EDT

(In reply to comment #0)

Are you sure that your expectation regarding arrays is correct?
I would say that end of a statement when placed on a new line should have the same indentation level as the beginning of that statement.

If im not mistaken this is already what you expect for functions

foo(function (){
*****	
})


So I would say 

EXPECTED:
--------------
<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        })
);
?>


To illustrate why I think this is the correct way to format I present a new test in which the closing parenthesis for the options array is placed on a new line:

--FILE--
<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        }|)
);
?>

EXPECTED:
--------------
<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        }
*****)
);
?>


With bracket notation that would be:


--FILE--
<?php
$schema = [
    'options' => [
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        }|]
];
?>

EXPECTED:
--------------
<?php
$schema = [
    'options' => [
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        }
*****]
];
?>





> 
> #2 Incorrect formatting of array statement
> 
> --FILE--
> <?php
> $schema = array(
>     'options' => array(
>         ServiceInstruction::VALIDATOR => function ($data)
>         {
>             return is_callable($data);
>         })|);
> ?>
> 
> EXPECTED:
> --------------
> <?php
> $schema = array(
>     'options' => array(
>         ServiceInstruction::VALIDATOR => function ($data)
>         {
>             return is_callable($data);
>         })
> *****);
> ?>
> 
> ACTUAL:
> --------------
> <?php
> $schema = array(
>     'options' => array(
>         ServiceInstruction::VALIDATOR => function ($data)
>         {
>             return is_callable($data);
>         })
> *****    );

Comment 9 Natalia Bartol 2013-06-12 08:36:20 EDT

Please take a look at the attached unit tests files, those asterisks (*****) may be misleading when you read them, they are placed only to make the difference visible.

Comment 10 exceptione 2013-06-12 10:37:37 EDT

(In reply to comment #9)
> Please take a look at the attached unit tests files, those asterisks (*****)
> may be misleading when you read them, they are placed only to make the
> difference visible.

I see now, sorry for the confusion.

Comment 11 Michal Niewrzal 2014-07-06 15:06:59 EDT

I tried to reproduce cases from Natalia's description with version PDT	3.3.0.201407051616 and it looks OK for me. Can anyone check it also?

Comment 12 Dawid Pakula 2014-07-06 15:08:50 EDT

In editor everything is ok, but I'm unable to put it as PDTT tests.

Comment 13 Michal Niewrzal 2014-07-06 15:44:04 EDT

Ok, right now I see the difference. First test case assume that after new line and bracket indentation empty line will have indentation +1 and now it is 0. Like this:

<?php
foo(function (){
****|
})
?>


Second case with array I think isn't covered. During test new indentation for this test case is level +1 but it should be level 0 like in test case. Actually it looks that indentation for this bracket is done with second array as base. For me it looks like this:

<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        })
    ); <- this should be not indented
?>

Comment 14 Michal Niewrzal 2014-08-07 05:33:21 EDT

I used previous patch that was reverted and apply all changes without removing extension point. I also fix some small problems which I found after applying changes.

Patch: https://git.eclipse.org/r/#/c/31173/

Comment 15 Dawid Pakula 2014-08-09 05:16:45 EDT

Merged : http://git.eclipse.org/c/pdt/org.eclipse.pdt.git/commit/?id=05a509cc3c38cbd6fd63a1971c55c45a2b71721e

Thank you Michal!

Comment 16 Sylvia Tancheva 2015-06-01 03:03:48 EDT

In case of this example:
<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        })|);
?>

When hitting enter I get:
<?php
$schema = array(
    'options' => array(
        ServiceInstruction::VALIDATOR => function ($data)
        {
            return is_callable($data);
        })
        );
?>
Reopen