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

Psalm level 2 #246

Merged
merged 13 commits into from
Mar 4, 2024
Merged

Psalm level 2 #246

merged 13 commits into from
Mar 4, 2024

Conversation

vjik
Copy link
Member

@vjik vjik commented Jan 21, 2024

Q A
Is bugfix?
New feature?
Breaks BC? ✔️

Copy link

codecov bot commented Jan 21, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 76.25%. Comparing base (6712de9) to head (caeaff0).

Files Patch % Lines
src/Command/DebugEventsCommand.php 0.00% 3 Missing ⚠️
src/Command/DebugContainerCommand.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #246      +/-   ##
============================================
+ Coverage     76.21%   76.25%   +0.03%     
+ Complexity      586      585       -1     
============================================
  Files            48       48              
  Lines          2031     2030       -1     
============================================
  Hits           1548     1548              
+ Misses          483      482       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vjik vjik requested a review from a team January 21, 2024 13:26
@vjik vjik added the status:code review The pull request needs review. label Jan 21, 2024
@@ -49,7 +50,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int
if ($input->hasOption('groups') && $input->getOption('groups')) {
$build = $this->getConfigBuild($config);
$groups = array_keys($build);
ksort($groups);
Copy link
Member

Choose a reason for hiding this comment

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

For what reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not need. array_keys returns already sorted array.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ksort is sorted by keys

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe here need to use sort instead ksort?

Copy link
Member

Choose a reason for hiding this comment

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

Ksort is sorted by keys

What?

There should be a sorted list.

Copy link
Member Author

@vjik vjik Jan 22, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh gotcha. You're right, there should be sort instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Added sort.

src/Command/DebugContainerCommand.php Show resolved Hide resolved
@@ -62,7 +61,6 @@
LoggerInterface::class => [LoggerInterfaceProxy::class, LogCollector::class],
EventDispatcherInterface::class => [EventDispatcherInterfaceProxy::class, EventCollector::class],
ClientInterface::class => [HttpClientInterfaceProxy::class, HttpClientCollector::class],
CacheInterface::class,
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems, code don't work with integer keys.

Copy link
Member

Choose a reason for hiding this comment

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

CacheInterface should be also proxied. Could you add it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, proxy and collector for CacheInterface not exist.

@vjik vjik requested a review from xepozz March 3, 2024 10:53
@xepozz xepozz merged commit b79ba7f into master Mar 4, 2024
19 checks passed
@xepozz xepozz deleted the psalm2 branch March 4, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants