-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Update code based on PHPStan report #87
Conversation
@@ -732,7 +732,7 @@ protected function calculateEaster($year, $timezone) | |||
if (extension_loaded('calendar')) { | |||
$easter_days = \easter_days($year); | |||
} else { | |||
$golden = (int)(($year % 19) + 1); // The Golden Number | |||
$golden = ($year % 19) + 1; // The Golden Number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall I had to typecast to integer as it was given incorrect results without the typecast. Did the unit tests run without errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo will always return an integer, so the typecast is not needed. PHPStan also find this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. Maybe my memory is failing on me, haha. Anyway, agree to remove the typecast.
'en_US' => $date->translations['en_US'] . ' Observed', | ||
'ja_JP' => '振替休日 (' . $date->translations['ja_JP'] . ')', | ||
], $substituteDay, $this->locale); | ||
$substituteHoliday = new Holiday('substituteHoliday:' . $shortName, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the if-clause means the substitute holiday is always added. I remember that sometimes it is not defined with the calculation (that's why the if-clause was there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the substitute holiday will always evaluate to true, so the condition is not needed. Can't find anywhere where $substituteDay can be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right.
No description provided.