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

[persistence] Implement HistoricItem.getInstant #17578

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joerg1985
Copy link
Contributor

This PR does implement the new HistoricItem.getInstant method from PR openhab/openhab-core#4384 for persistence implementations.

@joerg1985 joerg1985 requested review from a team, lujop and ssalonen as code owners October 16, 2024 18:55
@joerg1985 joerg1985 force-pushed the historic-get-instant branch 2 times, most recently from 9ac107d to 2f1c724 Compare October 17, 2024 16:18
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Oct 17, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Might have more comment,s just wanted to make a start with this.

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.
As i don't expect you to have tested all different persistences services and the the effects can be huge for users, i like to ask @jlaur to also look at this. For both his experience on persistence and datetime handling.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM as well, many thanks.

@ssalonen
Copy link
Contributor

Re dynamodb, there is quite extensive tests in CI

if running locally with correct env variables, it can also test against live dynamodb

of course if CI is run using UTC, it might not reveal all the bugs

@lsiepel
Copy link
Contributor

lsiepel commented Oct 22, 2024

@joerg1985 can you resolve the conflicts?

@joerg1985
Copy link
Contributor Author

@lsiepel i have rebased the branch and fixed the conflict.

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.

4 participants