[astro] Review Eclipses calculations#20077
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors eclipse calculations in the Astro binding by extracting common logic into reusable base classes, making the Eclipse model immutable and Instant-based, adding Moon eclipse icons, and introducing comprehensive unit tests for eclipse calculations.
Changes:
- Refactored eclipse calculations into a hierarchy (AstroCalc → EclipseCalc → MoonEclipseCalc/SunEclipseCalc) to eliminate code duplication
- Made Eclipse model immutable with Instant-based dates instead of mutable Calendar objects
- Added Moon eclipse icon set with three SVG variants (default, total, partial)
Reviewed changes
Copilot reviewed 22 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| EclipseCalcTest.java | New comprehensive unit tests for Sun and Moon eclipses using historical eclipse data |
| MoonCalcTest.java | Updated tests to use new API (getDistanceType(), getEclipses()) |
| moon_eclipse*.svg (3 files) | New icon resources for Moon eclipse visualization |
| MathUtils.java | Added frac() utility method for fractional part calculation |
| DateTimeUtils.java | Added GMST/LMST conversion methods, JD constants, and addDays() utility |
| AstroConstants.java | Added Earth radius, flattening, and prime meridian rate constants |
| Eclipse.java | Refactored to be immutable with Instant-based dates and internal EclipseData record |
| Moon.java | Changed to immutable eclipses field and removed deprecated getApogee()/getPerigee()/getDistance() methods |
| Sun.java | Renamed eclipse field to eclipses and made it final, removed setter |
| MoonPosition.java | Fixed NULL constant type from Position to MoonPosition |
| AstroCalc.java | New base class with shared eclipse calculation helper methods |
| EclipseCalc.java | Abstract class containing common eclipse calculation logic |
| MoonEclipseCalc.java | Moon-specific eclipse detection logic |
| SunEclipseCalc.java | Sun-specific eclipse detection logic |
| MoonCalc.java | Extensively refactored to extend AstroCalc and use new eclipse hierarchy |
| SunCalc.java | Refactored to use SunEclipseCalc and calculate eclipse elevations inline |
| AstroThingHandler.java | Simplified getPositionAt() to return non-nullable Position |
| MoonHandler.java | Updated to use new eclipse and distance APIs |
| SunHandler.java | Updated to use new eclipse API and removed setElevations() call |
| DailyJobMoon.java | Updated to schedule eclipse events using Instant instead of Calendar |
| DailyJobSun.java | Updated to schedule eclipse events using Instant instead of Calendar |
| AstroIconProvider.java | Added MOON_ECLIPSE_SET to supported icon sets |
| README.md | Documented new moon_eclipse icon set |
Comments suppressed due to low confidence (1)
bundles/org.openhab.binding.astro/src/main/java/org/openhab/binding/astro/internal/util/MathUtils.java:18
- Corrected grammar from 'Common used DateTime functions' to 'Commonly used DateTime functions'.
* Common used DateTime functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Gaël L'hopital <gael@lhopital.org>
Signed-off-by: Gaël L'hopital <gael@lhopital.org>
Signed-off-by: Gaël L'hopital <gael@lhopital.org>
lspiel code review adressed. Rebased and conflicts resolution Nadahar and Copilot code review Signed-off-by: clinique <gael@lhopital.org> Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
d0a2db9 to
4d5b6fd
Compare
Signed-off-by: Nadahar <Nadahar@users.noreply.github.com>
|
I tried to use GitHub to resolve the conflict - I might have caused more trouble than I solved, because it seems like GitHub made a merge commit out of it. I'm sorry for that, but since squashing is the usual practice in OH, it might not matter. I "always" rebase everything, so I have all but forgotten that merge commits exist (they are an evil mess). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 37 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nding/astro/internal/model/EclipseSet.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Gaël L'hopital <gael@lhopital.org>
* Instant and Immutable Eclipse Signed-off-by: Gaël L'hopital <gael@lhopital.org> Co-authored-by: Nadahar <Nadahar@users.noreply.github.com> Signed-off-by: Merlin10437 <152161717+Merlin10437@users.noreply.github.com>
Factorization of common Moon/Sun Eclipse parts.
Addition of Moon Eclipse Icon set
Eclipse model object now Instant based and Immutable
Unit testing for Eclipses added
Last part of PR #19910