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

[rector] [dead-code] Remove few ifs that are always true - part #2 #12700

Merged
merged 5 commits into from Oct 3, 2023

Conversation

TomasVotruba
Copy link
Contributor

Younger brother of #12693

@codecov
Copy link

codecov bot commented Sep 6, 2023

Codecov Report

Merging #12700 (f635008) into 5.x (d4ef86f) will increase coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                5.x   #12700   +/-   ##
=========================================
  Coverage     58.58%   58.58%           
+ Complexity    33610    33607    -3     
=========================================
  Files          2180     2180           
  Lines        101733   101730    -3     
=========================================
  Hits          59597    59597           
+ Misses        42136    42133    -3     
Files Changed Coverage Δ
...s/PluginBundle/Integration/AbstractIntegration.php 35.35% <ø> (ø)
...icCrmBundle/Integration/ConnectwiseIntegration.php 10.75% <0.00%> (+0.02%) ⬆️
...auticCrmBundle/Integration/SugarcrmIntegration.php 0.00% <0.00%> (ø)
...ignBundle/Executioner/Scheduler/EventScheduler.php 75.17% <100.00%> (-0.18%) ⬇️

... and 1 file with indirect coverage changes

@TomasVotruba TomasVotruba force-pushed the tv-rector-sets-2 branch 3 times, most recently from 501d9b9 to ba75ebc Compare September 6, 2023 15:49
@TomasVotruba TomasVotruba marked this pull request as ready for review September 6, 2023 15:55
@TomasVotruba
Copy link
Contributor Author

Ready for review ✔️

@TomasVotruba
Copy link
Contributor Author

TomasVotruba commented Sep 12, 2023

Rebased ✔️


This is good to go now 👍 Basically 2nd part of previous just merged PR :)

Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

The code changes make sense. I dug through the code if the removed if statements won't cause any issues or performance degradation but it all looks like it's handled in lower layers.

Adding the return types to public methods is a BC break but since these changes would go to a major release, we can get it merged. We were forced to add types by our dependencies in multiple places anyway so a couple more shouldn't cause any surprises.

Thank you Tomas! 👍

@escopecz escopecz added enhancement Any improvement to an existing feature or functionality refactoring The change does not change behavior but improves the code labels Oct 3, 2023
@escopecz escopecz added this to the 5.0-beta-1 milestone Oct 3, 2023
@escopecz escopecz merged commit 8ec1547 into mautic:5.x Oct 3, 2023
14 checks passed
@TomasVotruba TomasVotruba deleted the tv-rector-sets-2 branch October 3, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any improvement to an existing feature or functionality refactoring The change does not change behavior but improves the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants