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

feat: Introduce reference_spaces fixer #7386

Closed
wants to merge 2 commits into from

Conversation

greew
Copy link

@greew greew commented Oct 24, 2023

This pull request adds a new PhpCsFixer\Fixer\Whitespace\ReferenceSpacesFixer.

Based on #7303, this code

<?php
$bar = 'a';
$foo =& $bar;

will be changed to

<?php
$bar = 'a';
$foo = & $bar;

with a space between = and &.

This PR adds to possibility to handle the reference operator individually in the following cases:

  • assignments
    • by_reference: $foo = &$bar
    • by_assign: $foo =& $bar
    • single_space: $foo = & $bar
    • no_space: $foo =&$bar
  • function signatures
    • by_reference: function foo(&$bar) {...
    • single_space: function foo(& $bar) {...
  • use arguments
    • by_reference: $foo = function () use (&bar) {...
    • single_space: $foo = function () use (& bar) {...

@greew
Copy link
Author

greew commented Oct 24, 2023

Locally, I get the following error when running tests:

There was 1 failure:

1) PhpCsFixer\Tests\AutoReview\FixerFactoryTest::testFixerWithNoneDefaultPriorityIsTested
Fixers without default priority and without priority tests: "reference_spaces."
Failed asserting that an array is empty.

/app/tests/AutoReview/FixerFactoryTest.php:326

FAILURES!
Tests: 486, Assertions: 3918, Failures: 1.

I don't know if I should add the reference_spaces anywhere like FixerFactoryTest::getFixersPriorityGraph() or something like that?

*/
public const NO_SPACE = 'no_space';

/** @var array<'&'|287|288> */
Copy link
Member

@keradus keradus Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what 287 stands for?
(hint: numeric value of token ID may very across different PHP versions)

use T_... constant here instead

*/
public function getPriority(): int
{
return -33;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the failing test that you see is because you put a non-zero priority here. it is asking you to create an explicit priority testing case in tests/Fixtures/Integration/priority/ , and entry in getFixersPriorityGraph you mentioned


protected function createConfigurationDefinition(): FixerConfigurationResolverInterface
{
return new FixerConfigurationResolver([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for each of the option here - is any formatting of those preferred by PSR/PER ?

if so, is there a community using other possibilities?
if not - please drop those additional options

As PHP CS Fixer, we do not aim to cover each possible variant of style, but only the recommended / most popular ones, as each configuration option increase the maintenance cost and go against the purpose to spread the best recommendations.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both PSR and PER only mention the reference operator in regards to function signature. Nothing in regards to assignment. I would think that we could easily merge the function_signature and anonymous_function_use_block options into one.

After doing some searching I have not been able to find anything else than

  • assignment: $foo =& $bar or $foo = &$bar
  • signatures: function foo(&$bar)

I just added function signatures based on @Wirone comment in #7303

Would you say I should altogether remove the function_signature and anonymous... options and just handle assign by reference (either =& $bar or = &$bar) in this PR?

@@ -919,6 +919,9 @@ Whitespace
- `no_whitespace_in_blank_line <./whitespace/no_whitespace_in_blank_line.rst>`_

Remove trailing whitespace at the end of blank lines.
- `reference_spaces <./whitespace/reference_spaces.rst>`_
Copy link
Member

@keradus keradus Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before we go into the new fixer, we need to acknowledge that some users will not use new feature, but instead they will use only already existing features, that will lead to confusing style format.

Can I ask you, before we proceed with this new fixer, to first work on bugfix that would prevent existing rules from applying the wrong change?
If yes, please raise that as a separated PR.

I identified those 2 rules that are doing changes, but let's have it discussed here first:
#7303 (comment)

case 1:

   1) x.php (unary_operator_spaces)
 <?php
 
-$a =& $b;
+$a =&$b;

case 2:

   1) x.php (binary_operator_spaces)
 <?php
 
-$a =& $b;
+$a = & $b;

@coveralls
Copy link

Coverage Status

coverage: 94.616% (+0.01%) from 94.603% when pulling 184d918 on greew:add-reference-spaces-fixer into 25da939 on PHP-CS-Fixer:master.

Example #8
~~~~~~~~~~

With configuration: ``['anonymous_function_use_block' => 'by_reference']``.
Copy link
Member

@keradus keradus Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part is already covered by unary_operator_spaces rule.
Why introduce another rule doing the same? We shall avoid this.

@Wirone Wirone changed the title feat: New PhpCsFixer\Fixer\Whitespace\ReferenceSpacesFixer fixer feat: Introduce reference_spaces fixer Oct 25, 2023
@greew
Copy link
Author

greew commented Oct 26, 2023

I'll close this PR as #7388 fixes the issue.

@greew greew closed this Oct 26, 2023
@Wirone
Copy link
Member

Wirone commented Oct 27, 2023

Sorry if I wasted your time with wrong suggestion, @greew 🙁.

@greew
Copy link
Author

greew commented Oct 27, 2023

I was disappointed, but it was good practice learning more about PHP-CS-Fixer internals 😊

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

Successfully merging this pull request may close these issues.

None yet

4 participants