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

Simplify DateTimeType handling #17725

Merged
merged 82 commits into from
Dec 19, 2024
Merged

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Nov 9, 2024

This is a cleanup only and does not change functionality when openhab/openhab-core#3583 has first been merged. The core PR discards time-zone from DateTimeType, thus binding code can be simplified:

  • Instantiations from ZonedDateTime using either time-zone from TimeZoneProvider or ZoneId.systemDefault() have been simplified to use Instant. In some cases this entirely eliminated the need for TimeZoneProvider, which has then been removed.
  • Usages of deprecated methods toZone and toLocaleZone have been removed.
  • Usages of deprecated method getZonedDateTime() have been removed. In cases where time-zone is needed, getZonedDateTime(ZoneId.systemDefault()) is now explicitly called.

Related to openhab/openhab-core#3583

@jlaur jlaur added work in progress A PR that is not yet ready to be merged awaiting other PR Depends on another PR labels Nov 9, 2024
@jlaur jlaur force-pushed the datetimetype-instant branch 3 times, most recently from 86ec8a2 to d397b88 Compare November 11, 2024 19:29
@jlaur jlaur force-pushed the datetimetype-instant branch 8 times, most recently from 2a79387 to 9940eec Compare November 17, 2024 20:51
@jlaur jlaur mentioned this pull request Nov 18, 2024
@jlaur jlaur force-pushed the datetimetype-instant branch 11 times, most recently from 315137b to b3209c5 Compare November 24, 2024 22:36
@jlaur jlaur force-pushed the datetimetype-instant branch 4 times, most recently from d7d358e to adc97e5 Compare November 26, 2024 22:01
jlaur added 18 commits December 18, 2024 21:00
@jlaur jlaur force-pushed the datetimetype-instant branch from 8dc387a to a525c1c Compare December 18, 2024 20:01
@jlaur
Copy link
Contributor Author

jlaur commented Dec 19, 2024

Not merging it will make 4.3.x patches harder due to the amount of files and possible conflicts. WDYT?

Perhaps one way to mitigate this issue would be to merge it in the very beginning of the 5.0 development cycle and avoid squash-merging. When preserving all commits, it would be a simple matter of cherry-picking the commit for a specific binding before cherry-picking the patch commit. WDYT?

I have carefully crafted one commit per binding. This makes it easier for me to handle potential conflicts, and it also allowed me to quickly cherry-pick a single commit into #17872 for fixing a CI test. It has also helped me keeping a clean history by squashing commits when I had to do multiple iterations for a single binding.

Just a small reminder about this suggestion, did you consider it @lsiepel?

@lsiepel
Copy link
Contributor

lsiepel commented Dec 19, 2024

ps one way to mitigate this issue would be to merge it in the very beginning of the 5.0 development cycle and avoid squash-merging. When preserving all commits, it would be a simple matter of cherry-picking the commit for a specific binding before cherry-pickin

Yes, i think that is the best. As you where still adding changes, i waited. Does it mean this PR is finished now?

@jlaur
Copy link
Contributor Author

jlaur commented Dec 19, 2024

As you where still adding changes, i waited. Does it mean this PR is finished now?

Sorry about that - I found a few more places. 😉

In the last iteration, I was searching for "new DateTimeType(" looking for places where ZonedDateTime was used unneededly. Some of the results were quite hidden and I needed to open the specific bindings into my IDE for closer inspection, which then lead to a few more findings.

I probably missed a few, but I think the PR is ready to be merged now.

@lsiepel lsiepel merged commit 254f2f5 into openhab:main Dec 19, 2024
3 checks passed
@lsiepel lsiepel added this to the 5.0 milestone Dec 19, 2024
@jlaur jlaur deleted the datetimetype-instant branch December 19, 2024 11:12
@jlaur
Copy link
Contributor Author

jlaur commented Dec 19, 2024

@lsiepel - thanks for the review, I know it was quite a lot of files.

@lsiepel
Copy link
Contributor

lsiepel commented Dec 24, 2024

back ported commit for Network binding. (i can't set a patch anywhere, so i leave this message here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.