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

feat: add function to get day of omer #20

Merged
merged 4 commits into from
Jul 7, 2021
Merged

feat: add function to get day of omer #20

merged 4 commits into from
Jul 7, 2021

Conversation

zachweix
Copy link
Contributor

No description provided.

@zachweix
Copy link
Contributor Author

zachweix commented Jun 17, 2021

Closes #17

@zachweix
Copy link
Contributor Author

There are two failing tests, but I don't understand their messages

Copy link
Contributor

@tam5 tam5 left a comment

Choose a reason for hiding this comment

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

Can you please run composer lint? It will auto-format the code and fix the lint issues

Comment on lines 12 to 25
public function getOmerCount()
{
if ($this->jewishMonth == 8 && $this->jewishDay >= 16) {
return $this->jewishDay - 15;
}
if ($this->jewishMonth == 9) {
return $this->jewishDay + 15;
}
if ($this->jewishMonth == 10 && $this->jewishDay <= 5) {
return $this->jewishDay + 44;
}

return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return null when the count is not applicable rather than 0?

Also, what do you think about something like this instead:

$count = $this->diffInDays(
    static::firstDayOfPesach($this->jewishYear)
);

return $count > 0 && $count < 50 ? $count : null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but my logic was that the Omer days don't actually change from year to year, and it's slightly quicker and less resource intensive to just check the date than to create another object and compare the difference in days. In the end of the day it's your choice, but that's why I went this route

@zachweix
Copy link
Contributor Author

I can run lint on this soon, thanks for letting me know that I can do that

@zachweix
Copy link
Contributor Author

I ran lint on the code (and I still can't tell the difference), but one of the checks is still failing

@tam5 tam5 changed the title add function to get day of omer feat: add function to get day of omer Jun 29, 2021
@tam5 tam5 merged commit 591702a into zman-org:main Jul 7, 2021
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