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

Update code based on PHPStan report #87

Merged
merged 1 commit into from
Jan 5, 2018
Merged

Conversation

lukosius
Copy link
Contributor

@lukosius lukosius commented Jan 5, 2018

No description provided.

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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, [
Copy link
Member

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

Copy link
Contributor Author

@lukosius lukosius Jan 5, 2018

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.

https://travis-ci.org/azuyalabs/yasumi/jobs/325265019#L784

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right.

@stelgenhof stelgenhof merged commit 2ee91e6 into master Jan 5, 2018
@stelgenhof stelgenhof deleted the phpstan-code-fixes branch June 8, 2018 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants