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

Add support for displaying Daf Yomi #30628

Merged
merged 15 commits into from Feb 15, 2020
Merged

Add support for displaying Daf Yomi #30628

merged 15 commits into from Feb 15, 2020

Conversation

@moshekaplan
Copy link
Contributor

moshekaplan commented Jan 9, 2020

Description:

Add support for displaying the day's Daf Yomi to the jewish_calendar component.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@homeassistant

This comment has been minimized.

Copy link
Contributor

homeassistant commented Jan 9, 2020

Hi @moshekaplan,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@project-bot project-bot bot added this to Needs review in Dev Jan 9, 2020
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Jan 9, 2020

Hey there @tsvi, mind taking a look at this pull request as its been labeled with a integration (jewish_calendar) you are listed as a codeowner for? Thanks!

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jan 9, 2020
moshekaplan added a commit to moshekaplan/home-assistant.io that referenced this pull request Jan 9, 2020
Accompany doc update for home-assistant/core#30628
moshekaplan added 4 commits Jan 9, 2020
@fabaff fabaff changed the title jewish_calendar component: Add support for displaying Daf Yomi Add support for displaying Daf Yomi Jan 9, 2020
@tsvi

This comment has been minimized.

Copy link
Contributor

tsvi commented Jan 9, 2020

Hi @moshekaplan,

Thank you very much for your contribution.
Unfortunately, you will need to split up the code between the hdate package which is used by the Jewish calendar and the code in home assistant.

The homeassistant maintainers try to keep the non- homeassistant code to a minimum.

Please open a PR on royi1000/py-libhdate with the code for calculating the daf. (Yes, I know the code there is a bit a mess, help is more than welcome).

Once we've got that code working, I'll upload a new version to pypi which you can then use here.

It's a bit cumbersome, but it helps keeping the code in homeassistant clean.

Thank you,

Tsvi

Dev automation moved this from Needs review to Review in progress Jan 9, 2020
@moshekaplan

This comment has been minimized.

Copy link
Contributor Author

moshekaplan commented Jan 10, 2020

Submitted py-libhdate/py-libhdate#46 . Once the daf yomi code is merged in there, this code can be modified to use the updated library.

@tsvi

This comment has been minimized.

Copy link
Contributor

tsvi commented Jan 22, 2020

Hi @moshekaplan

I just published version 0.9.4 of hdate. You can now continue with this PR. Don't forget after updating the manifest to run gen_requirements_all.

Thanks,

Tsvi

@tsvi

This comment has been minimized.

Copy link
Contributor

tsvi commented Jan 22, 2020

I was a bit to trigger happy. Please take 0.9.5 which passes tox.

moshekaplan added 2 commits Jan 22, 2020
@moshekaplan

This comment has been minimized.

Copy link
Contributor Author

moshekaplan commented Jan 24, 2020

@tsvi : Could you review this again?

@tsvi

This comment has been minimized.

Copy link
Contributor

tsvi commented Jan 24, 2020

You also should update the tests and add a check for the daf yomi sensor in there as well.

moshekaplan added 3 commits Jan 30, 2020
@moshekaplan

This comment has been minimized.

Copy link
Contributor Author

moshekaplan commented Jan 30, 2020

@tsvi : We might be good to go. Can you review again?

@tsvi

This comment has been minimized.

Copy link
Contributor

tsvi commented Jan 31, 2020

Looks ok to me

@tsvi
tsvi approved these changes Jan 31, 2020
@moshekaplan

This comment has been minimized.

Copy link
Contributor Author

moshekaplan commented Feb 7, 2020

@fabaff : Could you please review this again?

Copy link
Member

springstan left a comment

LGTM ✌

Dev automation moved this from Review in progress to Reviewer approved Feb 12, 2020
@springstan springstan merged commit 7dff5e7 into home-assistant:dev Feb 15, 2020
10 checks passed
10 checks passed
CI Build #20200130.116 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.58%)
Details
codecov/project 94.63% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Feb 15, 2020
@lock lock bot locked and limited conversation to collaborators Feb 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.