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

GroupImportFixer - introduction #4005

Merged

Conversation

greeflas
Copy link
Contributor

@greeflas greeflas commented Sep 26, 2018

Hi guys! I just implemented feature #3143. Please check it and give me some feedback. Thanks in advance.

.gitignore Outdated Show resolved Hide resolved
@greeflas greeflas force-pushed the feature-group-import-fixer branch 4 times, most recently from a1f0de6 to a10cee5 Compare September 27, 2018 09:09
@kubawerlos
Copy link
Contributor

Can you add a test case with these imports:

use Foo\Bar;
use Foo\Baz;
use Foo\Baz\John\Doe;
use Foo\Baz\John\Smith;
use Foo\Baz\John\Smith\Junior;
use Foo\Baz\Johnny\Doe;
use Foo\Baz\Johnny\Smith;

?

Possibly other combinations.

Also I wonder what strategy should be also in such case:

use Foo\Bar;
use For\Baz\Lorem\Ipsum\Lets\Write\Some\More\Strings\One;
use For\Baz\Lorem\Ipsum\Lets\Write\Some\More\Strings\Two;

Should it group by Foo or by For\Baz\Lorem\Ipsum\Lets\Write\Some\More\Strings or make nested grouping (is that even valid syntax?)?

@greeflas
Copy link
Contributor Author

greeflas commented Mar 7, 2019

@kubawerlos

  1. Added test case with your imports.
  2. It will group by For\Baz\Lorem\Ipsum\Lets\Write\Some\More\Strings. I think in future we can implement different strategies for this :)

@greeflas
Copy link
Contributor Author

greeflas commented Mar 11, 2019

@keradus @SpacePossum @kubawerlos guys, can you check this PR, please? Is it OK?

@SpacePossum SpacePossum changed the title Feature #3143: Implement group_import fixer GroupImportFixer - introduction Mar 29, 2019
@SpacePossum
Copy link
Contributor

I think this new rule must be added to the (known) conflicts map here
single_import_per_statement => group_import

@SpacePossum
Copy link
Contributor

When testing:

[09:03 AM]-[possum@zoo1]-[~/work/PHP-CS-Fixer]-[git feature-group-import-fixer]
$ php php-cs-fixer fix --dry-run --rules=group_import -vvv src/Tokenizer/Transformer/NullableTypeTransformer.php
Loaded config default.
Using cache file ".php_cs.cache".
E
Legend: ?-unknown, I-invalid file syntax, file ignored, S-Skipped, .-no changes, F-fixed, E-error

Checked all files in 0.010 seconds, 12.000 MB memory used

Files that were not fixed due to errors reported during linting after fixing:
1) /home/possum/work/PHP-CS-Fixer/src/Tokenizer/Transformer/NullableTypeTransformer.php


[PhpCsFixer\Linter\LintingException]
PHP Parse error: syntax error, unexpected 'use' (T_USE), expecting identifier (T_STRING) or function (T_FUNCTION) or const (T_CONST) on line 15.


Applied fixers: group_import

[09:03 AM]-[possum@zoo1]-[~/work/PHP-CS-Fixer]-[git feature-group-import-fixer]
$ php -v
PHP 7.2.15-0ubuntu0.18.04.2 (cli) (built: Mar 22 2019 17:05:14) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.2.15-0ubuntu0.18.04.2, Copyright (c) 1999-2018, by Zend Technologies
with Xdebug v2.6.0, Copyright (c) 2002-2018, by Derick Rethans
[09:03 AM]-[possum@zoo1]-[~/work/PHP-CS-Fixer]-[git feature-group-import-fixer]
$ g log
commit 77a3f327897274ffdeb61bc9f7b8066c6696b01b (HEAD -> feature-group-import-fixer, greeflas/feature-group-import-fixer)
Author: greeflas <vldmr.kuprienko@gmail.com>
Date:   Mon Apr 1 22:29:49 2019 +0300

CR fixes
[09:03 AM]-[possum@zoo1]-[~/work/PHP-CS-Fixer]-[git feature-group-import-fixer]
$ g st
On branch feature-group-import-fixer
Your branch is up to date with 'greeflas/feature-group-import-fixer'.

nothing to commit, working tree clean

It seems the rule created:

<?php
namespace PhpCsFixer\Tokenizer\Transformer;

use PhpCsFixer\Tokeniz\{use PhpCsFixer\Tokenizer\{CT, Token, Tokens};

form:

namespace PhpCsFixer\Tokenizer\Transformer;

use PhpCsFixer\Tokenizer\AbstractTransformer;
use PhpCsFixer\Tokenizer\CT;
use PhpCsFixer\Tokenizer\Token;
use PhpCsFixer\Tokenizer\Tokens;

@greeflas
Copy link
Contributor Author

greeflas commented Apr 2, 2019

@SpacePossum Issue was fixed. It was related to rtrim() function. In this case rtrim('PhpCsFixer\Tokenizer\AbstractTransformer', '\\AbstractTransformer') function was returned wrong namespace PhpCsFixer\Tokeniz.

Also I added test case for this.

@basheerOct
Copy link

Wasn't it merged to Master?

@greeflas
Copy link
Contributor Author

greeflas commented Jul 8, 2019

@basheerOct not yet.
@SpacePossum any progress on this PR?

Copy link
Member

@julienfalque julienfalque left a comment

Choose a reason for hiding this comment

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

Please add an integration (priority?) test that proves this new fixer plays nicely with ordered_imports.

tests/Fixer/Import/GroupImportFixerTest.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
tests/Fixer/Import/GroupImportFixerTest.php Show resolved Hide resolved
@julienfalque
Copy link
Member

Friendly ping @greeflas. Would you have time to continue working on this PR?

@greeflas
Copy link
Contributor Author

@juliendufresne yeap, I will check your comments when will have a time.

@Okspen
Copy link

Okspen commented Jun 14, 2020

@greeflas Hey, I would like to have this fixer. Will you update your merge request?

@greeflas
Copy link
Contributor Author

@Okspen Yeap, waiting for @julienfalque response.

@Okspen
Copy link

Okspen commented Jul 6, 2020

@julienfalque & @SpacePossum Could you please review?

tests/Fixer/Import/GroupImportFixerTest.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/GroupImportFixer.php Outdated Show resolved Hide resolved
@greeflas greeflas requested a review from kubawerlos July 13, 2020 16:53
Copy link
Member

@julienfalque julienfalque left a comment

Choose a reason for hiding this comment

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

One last small issue and we're good 👍

@Okspen
Copy link

Okspen commented Aug 13, 2020

@SpacePossum, @julienfalque Can you please merge the request or leave comments what is else required to finish it? Thanks!

@julienfalque julienfalque added the RTM Ready To Merge label Aug 13, 2020
@hackel
Copy link

hackel commented Aug 13, 2020

This is probably out of scope for this PR at this point, but I would love to be able to configure which types of imports this fixer applies to. e.g. I would group function and constant imports, but not classes or traits. Just something to keep in mind.

@SpacePossum SpacePossum added this to the 2.17.0 milestone Sep 16, 2020
@SpacePossum SpacePossum merged commit 886b521 into PHP-CS-Fixer:master Sep 16, 2020
@SpacePossum SpacePossum removed the RTM Ready To Merge label Sep 16, 2020
@SpacePossum
Copy link
Contributor

Thanks @greeflas !

It has been almost 2 years and I'm sorry for the long wait. I'm very happy to have merge your PR and hope a lot of user will use it to their benefit!

@greeflas
Copy link
Contributor Author

@SpacePossum Hooray! I hope for it as well!

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

Successfully merging this pull request may close these issues.

9 participants