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.

Sign up for GitHub

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

Jump to bottom

CS: fully_qualified_strict_types - import_symbols #53244

Closed
wants to merge 3 commits into from
Closed

CS: fully_qualified_strict_types - import_symbols #53244

wants to merge 3 commits into from
+48 −31

Conversation

keradus
Copy link
Member

@keradus keradus commented Dec 27, 2023

Q A
Branch? 7.1
Bug fix? no
New feature? no
Deprecations? no
Issues Fix CS
License MIT

suggested in https://github.com/symfony/symfony/pull/53233/files#r1437244439

@carsonbot carsonbot added this to the 7.1 milestone Dec 27, 2023
@keradus keradus mentioned this pull request Dec 27, 2023
Merged
@keradus keradus changed the title 7.1 cs2 CS: fully_qualified_strict_types - import_symbols Dec 27, 2023
xabbuh
xabbuh requested changes Dec 27, 2023
View reviewed changes
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

failures look related to me

@keradus
Copy link
Member Author

keradus commented Dec 27, 2023

this PR serves a purpose to demonstrate "how the codebase would look alike if we would incorporate another Fixer rule", as one of members suggested to get it enabled.

Before investing any time into failure of this PR, I would like to get confirmation we want to go this path and enable the rule. Diff is massive and I do not want to invest time into investigation, if final conclusion would be that "we do not want this rule to be enabled".

What's your take on the rule itself?

@xabbuh
Copy link
Member

xabbuh commented Dec 27, 2023

importing the classes looks indeed like what we want to have in the codebase

@keradus
Copy link
Member Author

keradus commented Dec 27, 2023

I take the eagerness to use the rule.

I will covert PR to draft as it is not in merge-ready state.

@keradus keradus marked this pull request as draft December 27, 2023 23:18
@derrabus derrabus mentioned this pull request Dec 28, 2023
Closed
@Wirone
Copy link
Contributor

Wirone commented Dec 28, 2023

Related to PHP-CS-Fixer/PHP-CS-Fixer#7629 - this PR probably could prepare a Symfony codebase for that.

@Wirone Wirone mentioned this pull request Dec 29, 2023
Merged
keradus
keradus commented Mar 18, 2024
View reviewed changes
src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php
@@ -132,7 +133,7 @@ public function __construct(array $options, \Redis|Relay|\RedisCluster|null $red
if ($address = $sentinel->getMasterAddrByName($sentinelMaster)) {
[$host, $port] = $address;
}
} catch (\RedisException|\Relay\Exception $redisException) {
} catch (\RedisException|RelayException $redisException) {
Copy link
Member Author

@keradus keradus Mar 18, 2024 •
edited

Choose a reason for hiding this comment

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

looks like Psalm cannot handle this, should I bother by that and stop the PR or continue with it?
cc @xabbuh

also, reported here: vimeo/psalm#10840

@keradus keradus marked this pull request as ready for review March 18, 2024 19:20
@keradus keradus requested a review from xabbuh March 18, 2024 19:26
@keradus keradus mentioned this pull request Mar 23, 2024
Closed
@nicolas-grekas
Copy link
Member

Please let me close this one.
We're fine with doing this case by case.
Usually importing is the best way, but in some cases, inlining the FQCN provides more clarity to the codebase.

@keradus keradus deleted the 7.1_CS2 branch April 12, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wirone Wirone Wirone left review comments

@derrabus derrabus derrabus left review comments

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@welcoMattic welcoMattic Awaiting requested review from welcoMattic

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@jderusse jderusse Awaiting requested review from jderusse

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@keradus @xabbuh @Wirone @nicolas-grekas @derrabus @carsonbot