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

SingleTraitInsertPerStatement - fix formatting for multiline "use" #4710

Merged
merged 1 commit into from
Jan 18, 2020
Merged

SingleTraitInsertPerStatement - fix formatting for multiline "use" #4710

merged 1 commit into from
Jan 18, 2020

Conversation

kubawerlos
Copy link
Contributor

Fixes #4709

@kubawerlos kubawerlos changed the base branch from 2.16 to 2.15 December 23, 2019 16:27
@julienfalque julienfalque added this to the 2.15.6 milestone Dec 30, 2019
@SpacePossum SpacePossum added the RTM Ready To Merge label Jan 7, 2020
@GrahamCampbell
Copy link
Contributor

What does it do with the following example:

<?php
final class Example
{
    use Foo,
        Bar,
//         Bazz,
        Baz;
}

@kubawerlos
Copy link
Contributor Author

@GrahamCampbell now it does well

@GrahamCampbell
Copy link
Contributor

There seems to still be an issue:

image

The newline in between the use and public function is getting deleted.

@kubawerlos
Copy link
Contributor Author

@GrahamCampbell where is this happening? Because definitely not in SingleTraitInsertPerStatementFixer in this PR.

@GrahamCampbell
Copy link
Contributor

Could you add the following test case:

<?php

class Example
{
    use Foo, Bar;

    public function baz() {}
}

it should be fixed to:

<?php

class Example
{
    use Foo;
    use Bar;

    public function baz() {}
}

I think your code will actually fix it to:

<?php

class Example
{
    use Foo;
    use Bar;
    public function baz() {}
}

@kubawerlos
Copy link
Contributor Author

Added, through the expected result is different from each of your examples - we are not adding any new line if none between imports (as in very first test case), so this what would be:

<?php
class Example
{
    use Foo;use Bar;

    public function baz() {}
}

@GrahamCampbell
Copy link
Contributor

Right, so your fixer is fixing to the wrong thing, and then another fixer is breaking up the statements?

@GrahamCampbell
Copy link
Contributor

// cc @SpacePossum. This fixer is meant to fix to the Symfony and PSR-12 style, but seems to not do that?

@kubawerlos
Copy link
Contributor Author

Right, so your fixer is fixing to the wrong thing

WHAAAT? Are you kidding? See the issue it's fixing - there is a test case that covers this issue...

See the original tests - they are not meant to break the lines, only to do what the fixer describes to do, which is 'Each trait use must be done as single statement.' - STATEMENT, not a line.

This fixer is meant to fix to the Symfony and PSR-12 style

Since when? It seems like overthinking. We have a fixer to split the imports into separate lines since a long time ago, this fixer is meant to be run before it and @SpacePossum thought about it as he wrote this integration test: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/v2.16.1/tests/Fixtures/Integration/priority/single_trait_insert_per_statement,braces.test

@GrahamCampbell
Copy link
Contributor

See the original tests - they are not meant to break the lines,

Yes, I know. I am saying I think the common sense thing to do is to have the fixer split onto new lines, even though the original implementation didn't do this. This is why I have cced in the original fixer author.

@GrahamCampbell
Copy link
Contributor

Because of exactly the problem I am describing, with the example I provided. Trying to have separation of responsibilities just leads to it being harder to use, and producing output that is not desirable.

@GrahamCampbell
Copy link
Contributor

Though, maybe this is something we should discuss for after this PR. ;)

@GrahamCampbell
Copy link
Contributor

This PR itself looks fine.

@SpacePossum
Copy link
Contributor

This fixer is meant to fix to the Symfony and PSR-12 style,

No, it is meant/designed to make multiple use statements

Trying to have separation of responsibilities just leads to it being harder to use, and producing output that is not desirable.

I disagree, please provide more than an opinion and I might change my mind.

For now, lets leave this out of the PR, the only issue I've seen so far is that there is no priority test between this fixer and class_attributes_separation.

@SpacePossum
Copy link
Contributor

Thank you @kubawerlos.

SpacePossum added a commit that referenced this pull request Jan 18, 2020
…e "use" (kubawerlos)

This PR was squashed before being merged into the 2.15 branch (closes #4710).

Discussion
----------

SingleTraitInsertPerStatement - fix formatting for multiline "use"

Fixes #4709

Commits
-------

a982efc SingleTraitInsertPerStatement - fix formatting for multiline \"use\"
@SpacePossum SpacePossum merged commit a982efc into PHP-CS-Fixer:2.15 Jan 18, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Jan 18, 2020
@kubawerlos kubawerlos deleted the fix_SingleTraitInsertPerStatement_formatting branch January 19, 2020 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants