Skip to content

[DeadCode] RemoveDeadZeroAndOneOperationRector should not remove Constants#4910

Merged
TomasVotruba merged 3 commits intorectorphp:mainfrom
TwanVermeulen:patch-1
Sep 19, 2023
Merged

[DeadCode] RemoveDeadZeroAndOneOperationRector should not remove Constants#4910
TomasVotruba merged 3 commits intorectorphp:mainfrom
TwanVermeulen:patch-1

Conversation

@TwanVermeulen
Copy link
Copy Markdown
Contributor

I'm curious about your thoughts!
Sometimes we use constants that explain a certain number by words.

If the constant number does not change the outcome of a mathematical expression, it's currently removed by RemoveDeadZeroAndOneOperationRector. So for constants that have a value of 1 this would be the case.
I would prefer to keep the constant, since it's added to increase readability.

What do you think? I added a test case that currently fails, but should pass imho.

Thanks for your input

@TomasVotruba
Copy link
Copy Markdown
Member

Hi, the constants should not be removed indeed 👍

Could you send fix as well? Thanks

@TwanVermeulen
Copy link
Copy Markdown
Contributor Author

@TomasVotruba I added a fix. I do see some failed checks, but I'm not sure if they are related to my changes. Please let me know if there's any action required from my side.

@TwanVermeulen
Copy link
Copy Markdown
Contributor Author

@TomasVotruba all checks are green after merging latest main

Comment on lines +17 to +37

?>
-----
<?php

namespace Rector\Tests\DeadCode\Rector\Plus\RemoveDeadZeroAndOneOperationRector\Fixture;

class SkipConstants
{
public const ONE = 1;

public function run()
{
$value = 5 * self::ONE;
$value = 5 / self::ONE;
$value = self::ONE * 5;
$value = self::ONE / 5;
}
}

?>
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.

second part is unneeded for equal content

Suggested change
?>
-----
<?php
namespace Rector\Tests\DeadCode\Rector\Plus\RemoveDeadZeroAndOneOperationRector\Fixture;
class SkipConstants
{
public const ONE = 1;
public function run()
{
$value = 5 * self::ONE;
$value = 5 / self::ONE;
$value = self::ONE * 5;
$value = self::ONE / 5;
}
}
?>

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.

Indeed, I'll get this merged and fixup as the main work is done 👍

@TomasVotruba
Copy link
Copy Markdown
Member

@TwanVermeulen Thank you, this is good to go 👍 🙂

@TomasVotruba TomasVotruba merged commit 3ffd995 into rectorphp:main Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants