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

Bump astral to 2.1 #32270

Closed
wants to merge 74 commits into from
Closed

Bump astral to 2.1 #32270

wants to merge 74 commits into from

Conversation

alucryd
Copy link

@alucryd alucryd commented Feb 27, 2020

Proposed change

Bump astral to 2.1

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Astral 2.x introduces a new LocationInfo class, uses dataclasses and refactored a lot of code. Tried to use the new functions to the same extent as the old one to the best of my knowledge.

Lots of tests are failing on the dev HEAD, using tox -e py38, even without these changes so I'm not sure whether I can check that local tests pass. FWIW the sun component worked fine in my local testing.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

  • Documentation added/updated for [www.home-assistant.io][docs-repository]

If the code communicates with devices, web services, or third-party tools:

  • The [manifest file][manifest-docs] has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following [Integration Quality Scale][quality-scale]:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@probot-home-assistant
Copy link

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

@project-bot project-bot bot added this to Needs review in Dev Feb 27, 2020
@homeassistant
Copy link
Contributor

Hi @alucryd,

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!

@@ -164,7 +165,7 @@ def update_events(self, utc_point_in_time):
)
self.location.solar_depression = -10
self._check_event(utc_point_in_time, "dawn", PHASE_SMALL_DAY)
self.next_noon = self._check_event(utc_point_in_time, "solar_noon", None)
self.next_noon = self._check_event(utc_point_in_time, "noon", None)
Copy link
Member

Choose a reason for hiding this comment

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

are solar_noon and noon equivalent?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. Solar noon is when the sun is highest in the sky. Noon is 12:00.

Copy link
Author

Choose a reason for hiding this comment

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

The docstring for the noon function indicates: Calculates the solar noon (the time when the sun is at its highest point.) and reverse for midnight.

Copy link
Member

Choose a reason for hiding this comment

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

noon and midnight were renamed here:
sffjunkie/astral@7e06188

@MartinHjelmare MartinHjelmare changed the title bump astral to 2.1 Bump astral to 2.1 Feb 27, 2020
@alucryd alucryd requested a review from fabaff as a code owner February 28, 2020 08:51
@home-assistant home-assistant deleted a comment from homeassistant Feb 28, 2020
@stale
Copy link

stale bot commented Mar 29, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Mar 29, 2020
@alucryd
Copy link
Author

alucryd commented Mar 29, 2020

Guys, would you mind having a look at this PR? Would hate for it to be closed automatically...

@stale stale bot removed the stale label Mar 29, 2020
@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 8, 2020

Please rebase on latest dev branch and solve the merge conflict. If tests pass I think we can merge.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Apr 9, 2020

I think you need to rebase from latest dev branch, and remove any non related commits. The branch is now not clean.

@bdraco
Copy link
Member

bdraco commented Jun 21, 2020

This PR needs to be rebased as there are unrelated changes.

https://developers.home-assistant.io/docs/development_catching_up/

@alucryd
Copy link
Author

alucryd commented Jun 21, 2020

There's no point in wasting time on this until upstream astral has been fixed in places like Norway.

@frenck frenck added the waiting-for-upstream We're waiting for a change upstream label Jun 21, 2020
@stale
Copy link

stale bot commented Jul 25, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Jul 25, 2020
@MartinHjelmare
Copy link
Member

I think we can close here until the library is fixed.

@alucryd if you close, you can reopen yourself and we keep the PR thread history intact.

@stale stale bot removed the stale label Jul 25, 2020
@alucryd
Copy link
Author

alucryd commented Jul 28, 2020

@MartinHjelmare Sure, let's do that.

@alucryd alucryd closed this Jul 28, 2020
Dev automation moved this from Review in progress to Cancelled Jul 28, 2020
@amitfin
Copy link
Contributor

amitfin commented Nov 3, 2020

@alucryd, IIUC, this PR seems to be blocked because of sffjunkie/astral#34. However, that issue was opened on Jun 5, 2019, while astral version 2.0 was released on Feb 10, 2020. So, this issue is not a regression of a newer version of the library, as the issue was opened against astral version 1.10.1, which is the version currently being used by HA. Furthermore, I believe that sffjunkie/astral#34 was actually fixed in astral version 2.0, which is another good reason to revive and push forward this PR.

(Context: I'm trying to push for upgrading astral version in HA because we have a dependency on the newer astral lib with HA's "jewish_calendar" integration and its "hdate" underline library.)

@alucryd
Copy link
Author

alucryd commented Nov 9, 2020

I'll try to get back to this during the week, but last time I tried astral 2.1 it broke every single unit test involving hardcoded sun positions and it wasn't just in arctic regions. Will see if 2.2 fares better.

@amitfin
Copy link
Contributor

amitfin commented Feb 8, 2021

@alucryd, did you give it a try? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet