Skip to content

refactor: add FullyQualifiedNameAnalyzer#8048

Merged
kubawerlos merged 3 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:add_FullyQualifiedNameAnalyzer
Mar 4, 2025
Merged

refactor: add FullyQualifiedNameAnalyzer#8048
kubawerlos merged 3 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:add_FullyQualifiedNameAnalyzer

Conversation

@kubawerlos
Copy link
Copy Markdown
Member

No description provided.

@kubawerlos kubawerlos enabled auto-merge (squash) May 26, 2024 21:02
@coveralls
Copy link
Copy Markdown

coveralls commented May 26, 2024

Coverage Status

coverage: 94.924% (-0.01%) from 94.938%
when pulling 12136be on 6b7562617765726c6f73:add_FullyQualifiedNameAnalyzer
into 5db535a on PHP-CS-Fixer:master.

Wirone
Wirone previously requested changes May 26, 2024
Copy link
Copy Markdown
Member

@Wirone Wirone left a comment

Choose a reason for hiding this comment

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

Did you take a look at FullyQualifiedStrictTypesFixer? I believe some logic from there could be a part of this analyzer, or the other way around - could take advantage of what was introduced here (like resolveSymbol()).

PS. in terms of import types - I know the refactored logic supported only classy imports, but I really believe that introducing new analyzer dedicated for FQCN resolving really should provide full support for all types of imports, as we already know that current implementation (prior to this PR) is buggy / not complete. This PR is great anyway, as this reduces redundancy, but can be even better 🙂.

Comment thread tests/Tokenizer/Analyzer/FullyQualifiedNameAnalyzerTest.php
Comment thread tests/Tokenizer/Analyzer/FullyQualifiedNameAnalyzerTest.php Outdated
Comment thread src/Tokenizer/Analyzer/FullyQualifiedNameAnalyzer.php
@Wirone Wirone added kind/refactoring topic/fqcn Fully Qualified Class Name usage and conversions labels May 26, 2024
@kubawerlos
Copy link
Copy Markdown
Member Author

@Wirone updated to your suggestions. I did not touch FullyQualifiedStrictTypesFixer as it would mess #7705 - I'll leave that for you 😄

/**
* @param NamespaceUseAnalysis::TYPE_* $importType
*/
public function getFullyQualifiedName(string $name, int $indexInNamespace, int $importType): string
Copy link
Copy Markdown
Contributor

@mvorisek mvorisek May 28, 2024

Choose a reason for hiding this comment

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

see #7724 and #7679 and #7705

All such logic must be implemented by this PR/FullyQualifiedNameAnalyzer class otherwise the result will not be always correct.

Then the resolving logic should be used in FullyQualifiedStrictTypesFixer and all existing tests must pass.

Also, the resolving should be fast and some cached-based uses lookup should be supported probably (verify the performance on some large large codebase with a lot of imports by running the fixed with FullyQualifiedStrictTypesFixer only).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This PR is not touching FullyQualifiedStrictTypesFixer and I'd like to keep it, and handle it separately, after #7705 is merged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally I think that caching and optimising can be done is separate PR as currently we don't have it too. Let's don't add features during refactor. When it comes to FullyQualifiedStrictTypesFixer I also would like to see it here, but I am open for doing it later too.

@kubawerlos when it comes to 7704/7705 it's been 4 months without any response from the core team, so we don't even know which approach is welcome 🤷. I would happily rebase my PR if it's the chosen way, but I am not going to work on it unless the decision is made.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So, since #7704 was closed, we can move forward with #7705 and this one, and then refactor FullyQualifiedStrictTypesFixer to use FullyQualifiedNameAnalyzer, right?

@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch 2 times, most recently from c32dfc0 to 1ebaff1 Compare August 7, 2024 21:31
@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch from 1ebaff1 to 65ea959 Compare August 29, 2024 18:43
@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch from 65ea959 to af61139 Compare September 28, 2024 06:49
@kubawerlos kubawerlos requested a review from Wirone September 28, 2024 06:49
@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch 2 times, most recently from 0a1783d to bf6989b Compare September 29, 2024 19:35
@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch from 5027427 to 23d7dc8 Compare January 20, 2025 15:33
@kubawerlos kubawerlos force-pushed the add_FullyQualifiedNameAnalyzer branch from cd7199f to 086fdb1 Compare February 24, 2025 09:17
@kubawerlos kubawerlos dismissed Wirone’s stale review March 4, 2025 13:53

Requested over months ago.

@kubawerlos kubawerlos merged commit 2adfc03 into PHP-CS-Fixer:master Mar 4, 2025
@kubawerlos kubawerlos deleted the add_FullyQualifiedNameAnalyzer branch March 4, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactoring topic/fqcn Fully Qualified Class Name usage and conversions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants