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.

Sign up for GitHub

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

Jump to bottom

Provide time.toInstant method #413

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Provide time.toInstant method #413

merged 3 commits into from
Jan 15, 2025

Conversation

jlaur
Copy link
Contributor

@jlaur jlaur commented Jan 5, 2025 •
edited
Loading

This provides a convenience method for constructing a time.toInstant from various input types, similar to time.toZDT.

The main purpose and motivation is to simplify this:

var startTime = time.toZDT(lastUpdateItem).toInstant();
var endTime = time.Instant.now();

to the more straight-forward (and faster):

var startTime = time.toInstant(lastUpdateItem);
var endTime = time.toInstant();

Please note I have deliberately left out a few conversions provided by time.toZDT which I consider slightly misleading and/or bloated, for example considering a number to be a number of milliseconds to be added to now. This can be achieved by directly using the js-joda classes, e.g. time.Instant.now().plusMillis(500) which to me seems more readable than time.toInstant(500).

I was in doubt whether to even add time.toInstant() (without parameter) in favor of using time.Instant.now(), but it seems so commonly used for time.toZDT that I decided to leave it in for first review iteration to be discussed. I also provided those operations working directly on items, although I also have doubts about DecimalType and QuantityType.

Testing

var zoned = time.toZDT(items.TestDateTime);
var instant = time.toInstant(items.TestDateTime);
console.log("item: " + instant.toString());
console.log("instant: " + time.toInstant(instant).toString());
console.log("now: " + time.toInstant().toString());
console.log("zoned: " + time.toInstant(zoned).toString());
console.log("string: " + time.toInstant('2024-10-31T20:00:02Z').toString());
console.log("item string: " + time.toInstant(items.TestInstantString));

console.log("zoned item quantity: " + time.toZDT(items.TestTime));
console.log("zoned item decimal: " + time.toZDT(items.TestNumber));

Output:

2025-01-05 23:12:23.749 [INFO ] [enhab.automation.script.file.test.js] - item: 2024-10-31T20:00:01Z
2025-01-05 23:12:23.752 [INFO ] [enhab.automation.script.file.test.js] - instant: 2024-10-31T20:00:01Z
2025-01-05 23:12:23.755 [INFO ] [enhab.automation.script.file.test.js] - now: 2025-01-05T22:12:23.754Z
2025-01-05 23:12:23.757 [INFO ] [enhab.automation.script.file.test.js] - zoned: 2024-10-31T20:00:01Z
2025-01-05 23:12:23.764 [INFO ] [enhab.automation.script.file.test.js] - string: 2024-10-31T20:00:02Z
2025-01-05 23:12:23.775 [INFO ] [enhab.automation.script.file.test.js] - item string: 2024-10-31T12:00:01Z
2025-01-05 23:12:23.778 [INFO ] [enhab.automation.script.file.test.js] - zoned item quantity: 2025-01-06T01:13:23.776+01:00[Europe/Copenhagen]
2025-01-05 23:12:23.781 [INFO ] [enhab.automation.script.file.test.js] - zoned item decimal: 2025-01-05T23:12:33.779+01:00[Europe/Copenhagen]

Resolves #412

@jlaur jlaur force-pushed the 412-time-instant branch 3 times, most recently from d93cb68 to d02bb08 Compare January 6, 2025 22:12
@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2025 •
edited
Loading

I was in doubt whether to even add time.toInstant() (without parameter) in favor of using time.Instant.now(), but it seems so commonly used for time.toZDT that I decided to leave it in for first review iteration to be discussed. I also provided those operations working directly on items, although I also have doubts about DecimalType and QuantityType.

@florian-h05 - what is your opinion here? I think the conversions from Java and openHAB types (like native Date, Java Instant, OH DateTimeType and OH item) are the ones that are most convenient, while other operations might be better left for the js-joda implementation (like time.ZonedDateTime.toInstant(), time.Instant.now()). It's easy to add more cases later, but very hard to remove anything later, once it has been introduced.

@florian-h05
Copy link
Contributor

The conversion from from Java and openHAB types clearly is a must-have, and I would also opt to add the time.toInstant() = time.Instant.now() functionality, I am really used to the convenience of just calling time.toZDT() that it would be annoying to have toInstant behave different here.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2025

The conversion from from Java and openHAB types clearly is a must-have, and I would also opt to add the time.toInstant() = time.Instant.now() functionality, I am really used to the convenience of just calling time.toZDT() that it would be annoying to have toInstant behave different here.

Okay, let's leave that then. So you'd be okay with removing DecimalType and QuantityType support from _convertItemRawStateToInstant?

@florian-h05
Copy link
Contributor

So you'd be okay with removing DecimalType and QuantityType support from _convertItemRawStateToInstant?

Yes, those conversions always seemed weird, but keep in mind that this is still a breaking change.

@jlaur
Copy link
Contributor Author

jlaur commented Jan 14, 2025

Yes, those conversions always seemed weird, but keep in mind that this is still a breaking change.

I would only remove them from the new time.toInstant, i.e. not introduce them at all. 🙂

@florian-h05
Copy link
Contributor

Okay in that case, no problem 👍

@jlaur
Provide time.toInstant method
9a4476d 
Resolves openhab#412

Signed-off-by: Jacob Laursen <[email protected]>
@jlaur jlaur marked this pull request as ready for review January 14, 2025 20:20
@jlaur jlaur requested a review from a team as a code owner January 14, 2025 20:20
florian-h05
florian-h05 requested changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

Apart from two documentation comments, looks good to me!

README.md Outdated Show resolved Hide resolved
src/time.js Show resolved Hide resolved
@florian-h05 florian-h05 added the enhancement New feature or request label Jan 14, 2025
@florian-h05 florian-h05 added this to the to be released milestone Jan 14, 2025
@jlaur
Clean up documentation
024b6ed 
Signed-off-by: Jacob Laursen <[email protected]>
florian-h05
florian-h05 approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

One minor typo left. Please refrain from further force pushes.

README.md Outdated Show resolved Hide resolved
jlaur
jlaur commented Jan 15, 2025
View reviewed changes
README.md Outdated Show resolved Hide resolved
@jlaur
Add star before Type in table
6e26bca 
Signed-off-by: Jacob Laursen <[email protected]>
@florian-h05 florian-h05 merged commit 7edd016 into openhab:main Jan 15, 2025
4 checks passed
@jlaur jlaur deleted the 412-time-instant branch January 15, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-h05 florian-h05 florian-h05 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
to be released
Development

Successfully merging this pull request may close these issues.

Extend support for time.Instant
2 participants
@jlaur @florian-h05