Skip to content
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

operators_spaces brakes array double arrows #2151

Closed
damnedest opened this issue Aug 19, 2016 · 17 comments
Closed

operators_spaces brakes array double arrows #2151

damnedest opened this issue Aug 19, 2016 · 17 comments

Comments

@damnedest
Copy link

damnedest commented Aug 19, 2016

Since 1.11.7 operators_spaces fixes arrays double arrow.

Before

<?php
array(
    'value'             => null,
    'values'            => null,
    'default_value'     => null,
    'required'          => false,
    'transformations'   => $this->getDefaultTransformations(),
);

After

<?php
array(
    'value' => null,
    'values' => null,
    'default_value' => null,
    'required' => false,
    'transformations' => $this->getDefaultTransformations(),
);

I think that operator fixer should not affect arrays since aligment in arrays is something more complex.

@keradus
Copy link
Member

keradus commented Aug 19, 2016

ref: may or may not be connected with not-fully prepared report #2135

Please, provide CLI command you are using (with all args), config file and PHP version.

@GrahamCampbell
Copy link
Contributor

Not related to that issue, but might be to my other one.

@GrahamCampbell
Copy link
Contributor

I'll close up all my garbage bug reports and replace them with real ones soon, I promise. :)

@damnedest
Copy link
Author

I started fixer only with operators_spaces, so he gives problems.

@keradus
Copy link
Member

keradus commented Aug 20, 2016

Too many spaces around operators are trimmed thanks to #2092

If you want to have them aligned, use align_double_arrow fixer.

D:\GIT\fork\PHP-CS-Fixer ((detached from v1.11.7))
λ php php-cs-fixer --version
PHP CS Fixer version 1.11.7 by Fabien Potencier

D:\GIT\fork\PHP-CS-Fixer ((detached from v1.11.7))
λ php php-cs-fixer fix investigate\ --dry-run --diff --fixers=operators_spaces,align_double_arrow
   1) test.php
      ---------- begin diff ----------
      --- Original
+++ New
@@ @@
 <?php
 array(
-    'value'             => null,
-    'values'            => null,
-    'default_value'     => null,
-    'required'          => false,
-    'transformations'   => $this->getDefaultTransformations(),
+    'value'           => null,
+    'values'          => null,
+    'default_value'   => null,
+    'required'        => false,
+    'transformations' => $this->getDefaultTransformations(),
 );


      ---------- end diff ----------

Checked all files in 1.374 seconds, 6.000 MB memory used

@damnedest
Copy link
Author

But I don't want to realign my arrays. operators_spaces is doing some job with arrays witch it shouldn't do.

@keradus
Copy link
Member

keradus commented Aug 21, 2016

why you want to have extra 2 spaces?

@damnedest
Copy link
Author

damnedest commented Aug 21, 2016

Basically problem is that this fixer brakes more complex aligments like this:

'phone' => array(
    'title'     => 'Phone',
    'type'      => 'text',
),
'url_id' => array(
    'title'     => 'Url ID',
    'type'      => 'text',
    'lower'     => true,
    'trim'      => true,
    'regexp'    => '#^[a-z_0-9-]+$#i',
    'computed'  => true,
),

All I want is that this operator won't affect arrays, because for align arrays we have "align_double_arrow". Maybe I'm wrong.

@keradus
Copy link
Member

keradus commented Aug 22, 2016

It's hard to have fixer for binary operators, and then say fix all operators except =>, why not except || as well? or +, or anything? hardcoded exception is tricky to explain to users and maintain.

For aligning we have separated fixer, as we don't want to align all operators (and aligning for => or = has different logic).

Why this is broken btw?

keradus@Lap:~/github/PHP-CS-Fixer$ php php-cs-fixer fix --fixers=operators_spaces,align_double_arrow --dry-run --diff test.php 
Loaded config from "/home/keradus/github/PHP-CS-Fixer/.php_cs".
   1) /home/keradus/github/PHP-CS-Fixer/test.php
      ---------- begin diff ----------
      --- Original
      +++ New
      @@ @@
       <?php

       $arr = array(
           'phone' => array(
      -        'title'     => 'Phone',
      -        'type'      => 'text',
      +        'title' => 'Phone',
      +        'type'  => 'text',
           ),
           'url_id' => array(
      -        'title'     => 'Url ID',
      -        'type'      => 'text',
      -        'lower'     => true,
      -        'trim'      => true,
      -        'regexp'    => '#^[a-z_0-9-]+$#i',
      -        'computed'  => true,
      +        'title'    => 'Url ID',
      +        'type'     => 'text',
      +        'lower'    => true,
      +        'trim'     => true,
      +        'regexp'   => '#^[a-z_0-9-]+$#i',
      +        'computed' => true,
           ),
       );


      ---------- end diff ----------

Checked all files in 0.092 seconds, 6.000 MB memory used

Also, if really needed, you could prepare a PR that will allow operators_spaces to not fix some operators (so any configured operators, not only =>).

@GrahamCampbell
Copy link
Contributor

This is only happening due to the bug causing it to be unaligned first...

@GrahamCampbell
Copy link
Contributor

Turn off both align fixers and it still gets unaligned.

@keradus
Copy link
Member

keradus commented Aug 22, 2016

operators_spaces is unaligning any operators

@GrahamCampbell
Copy link
Contributor

operators_spaces is unaligning any operators

That seems wrong?

@keradus
Copy link
Member

keradus commented Aug 22, 2016

I see nothing wrong about it.

@GrahamCampbell
Copy link
Contributor

It's unaligning double arrow, and that shouldn't be happening. That case should be excluded, or the double arrow unalign fixer should be removed, and this one replaced with configuration to select operators to unalign.

@keradus
Copy link
Member

keradus commented Aug 22, 2016

or the double arrow unalign fixer should be removed

👍, I was already thinking about this, but only for 2.x branch


configuration -> 👍

@GrahamCampbell
Copy link
Contributor

👍

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

No branches or pull requests

3 participants