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

Add NoUnneededImportAliasFixer #4498

Closed
wants to merge 22 commits into from

Conversation

JakeFr
Copy link

@JakeFr JakeFr commented Aug 9, 2019

Hi
First fixer, try to implement #4492

README.rst Outdated Show resolved Hide resolved
*/
public function getPriority()
{
// should be run after the SingleImportPerStatementFixer (for fix separated use statements as well) and NoUnusedImportsFixer (just for save performance) and NoLeadingImportSlashFixer
Copy link
Member

Choose a reason for hiding this comment

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

IMO this new fixer should be able to deal with grouped imports in case single_import_per_statement is not enabled. This would remove the priority issue. Same with no_leading_import_slash.

Copy link
Author

Choose a reason for hiding this comment

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

Sadly NamespaceUsesAnalyzer skips group and multiple use

Copy link
Author

Choose a reason for hiding this comment

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

Add support for multiple use (not group) with an extra argument to avoid side effect

src/Fixer/Import/NoUnneededAliasFixer.php Outdated Show resolved Hide resolved
src/Fixer/Import/NoUnneededAliasFixer.php Outdated Show resolved Hide resolved
@JakeFr JakeFr changed the title Add NoUnneededAliasFixer Add NoUnneededImportAliasFixer Aug 12, 2019
@JakeFr
Copy link
Author

JakeFr commented Aug 19, 2019

Look good to me now but I don't understand red build

@keradus
Copy link
Member

keradus commented Aug 24, 2019

Hi @JakeFr !
First of all, thanks for your contribution!

I did not had a chance yet to look into your PR, but I take my chances to understand why Travis is red - I can now confirm that it's not related to changes on your PR, but to Travis unattended update of some libraries. I tried to fix it here: #4516

public function getDefinition()
{
return new FixerDefinition(
'Remove unneeded alias in `use` clauses.',
Copy link
Member

Choose a reason for hiding this comment

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

reading this sentence, i don't know what unneeded means.
is baz as baz only case? then, audience of this rule is very limited

Copy link
Author

Choose a reason for hiding this comment

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

it's unneeded when the alias is equal to the last part of the fqcn
same with function and const imports
I found some import in big legacy project
not found in recent projects with modern IDE


// Multiple imports on one line:
// use My\Full\Classname as Another, My\Full\NSname;
// TODO: How to support these:
Copy link
Member

Choose a reason for hiding this comment

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

i guess this is not yet resolved?

Copy link
Author

Choose a reason for hiding this comment

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

not resolved, NamespaceUseAnalysis wont fit for group import
start and end tokens do not apply as the use is not in one piece

README.rst Outdated Show resolved Hide resolved
@JakeFr
Copy link
Author

JakeFr commented Oct 3, 2019

LGTM now

$this->doTest($expected, $input);
}

public function provideFixCases()
Copy link
Member

Choose a reason for hiding this comment

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

Please add test cases with grouped imports.

private function removeAlias(Tokens $tokens, NamespaceUseAnalysis $declaration)
{
$asIndex = $tokens->getNextTokenOfKind($declaration->getStartIndex(), [[T_AS]]);
if (null === $asIndex || $asIndex > $declaration->getEndIndex()) {
Copy link
Member

Choose a reason for hiding this comment

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

This method is only called if the import uses an alias so this should never happen.

@thdebay
Copy link

thdebay commented Dec 22, 2021

Is this PR still relevant? It addresses a feature I'd very much like to see in a future release. If there is some work remaining, I'd be happy to contribute

@JakeFr
Copy link
Author

JakeFr commented Dec 25, 2021

Hi,
I almost forget this PR.
I didn't success to fully support multiple use with group

use some\namespace\{ClassA, ClassB as ClassB, ClassC as C};
=>
use some\namespace\{ClassA, ClassB, ClassC as C};

And as it was mentioned the scope is very limited, this poc was used in a big legacy project with php 5 and "old generation" IDE.
I didnt find those erroneous use in other project since.

SpacePossum added a commit that referenced this pull request Feb 5, 2022
This PR was merged into the master branch.

Discussion
----------

NoUnneededImportAliasFixer - Introduction

replaces #4498

Commits
-------

8ea666d NoUnneededImportAliasFixer - Introduction
@SpacePossum
Copy link
Contributor

replaced by #6267 , closing as such, thanks @JakeFr for initial work and others for the reviews 👍

@SpacePossum SpacePossum closed this Feb 5, 2022
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.

None yet

5 participants