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

[time] Time Utils #101

Merged
merged 6 commits into from
May 13, 2022
Merged

[time] Time Utils #101

merged 6 commits into from
May 13, 2022

Conversation

rkoshak
Copy link
Contributor

@rkoshak rkoshak commented Mar 1, 2022

[time] Time Utils

Description

Implements a number of utilities for working with date times.

  • time.toZDT(): converts almost anything that can reasonably be converted to a date time to a ZonedDateTime. See the docs for the rules used when converting (e.g. a number is added to now as milliseconds, '1:23 PM' returns a date time with today's date and 13:23:00 for the time, etc.).
  • zdt.toToday(): returns a new time.ZonedDateTime with zdt's time and today's date, accounting for DST changes.
  • zdt.betweenTimes(start, end): returns true if zdt's time is between the times (ignoring date) represented by start and end. If the end time is before start, the time range is assumed to span midnight. start and end can be anything supported by time.toZDT(), for example now.betweenTimes(items.getItem('Sunset'), '05:00');.
  • zdt.isClose(otherZDT, maxDuration): returns true if the difference between zdt and otherZDT is within maxDuration.

Testing

I used the following in a MainUI Script to test this library .

var {testUtils} = require('openhab_rules_tools');
var {DateTimeType, DecimalType, QuantityType} = require('@runtime');
var OHItem = Java.type('org.openhab.core.items.Item');
var javaZDT = Java.type('java.time.ZonedDateTime');
var javaDur = Java.type('java.time.Duration');
var testItems = [];
var veryClose = time.Duration.ofMillis(30);
var prefix = 'TestToZDT';

var exceptionExpected = (runme) => {
  try {
    runme();
  }
  catch(e) {
    return true;
  }
  return false;
}

var testOhType = (name, type, state) => {
  items.replaceItem(name, type);
  testUtils.sleep(100);
  items.getItem(name).postUpdate(state);
  testUtils.sleep(100);
  testUtils.assert(items.getItem(name).state == state.toString());
  testItems.push(name);
}

try {
  console.info('Starting toZDT Tests')

  var testNum = 0;

  // Test 1: undefined or null
  console.info("Executing Test" + (++testNum));
  var now = time.ZonedDateTime.now();
  testUtils.assert(now.isClose(time.ZonedDateTime.now(), veryClose), 
                   'Test 1: Failed to convert undefined to now');

  // Test 2: passthrough
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(now) === now,
                  'Test 2: Failed to passthrough time.ZDT');

  // Test 3: Java ZDT
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(javaZDT.now()).isClose(time.toZDT(), veryClose),
                  'Test 3: Failed to convert Java ZDT');

  // Test 4: DateTimeType
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(new DateTimeType()).isClose(time.toZDT(), veryClose),
                  'Test 4: Failed. to convert DateTimeType');

  // Test 5: JavaScript Date
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(new Date()).isClose(time.toZDT(), veryClose),
                  'Test 5: Failed to convert JS Date');

  // Test 6: JavaScript Number
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(10).isClose(time.toZDT().plus(veryClose), veryClose),
                  'Test 6: Failed. to convert number');

  // Test 7: Java Duration
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(javaDur.ofMillis(10)).isClose(time.toZDT(10), veryClose),
                  'Test 7: Failed to convert a java Duration');

  // Test 8: time.Duration
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(veryClose).isClose(time.toZDT(10), veryClose),
                  'Test 8: Failed to convert time.Duration');

  // Test 9: DecimalType
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(new DecimalType(10)).isClose(time.toZDT(20), veryClose),
                  'Test 9: Failed to convert DecimalType');

  // Test 10: QuantityType
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT(new QuantityType('10 s')).isClose(time.toZDT(10000), veryClose),
                  'Test 10: Failed. to convert QuantityType');

  // Test 11: Unsupported units
  console.info("Executing Test" + (++testNum));
  testUtils.assert(exceptionExpected(() => time.toZDT(new QuantityType(10, '°F'))),
                  'Test 11: Failed to throw exception on unsupportted units');

  // Test 12: ISO 8601 String
  // TODO
  console.info("Executing Test" + (++testNum));

  // Test 13: 24 time
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT('13:21').isClose(time.toZDT().withHour(13).withMinute(21).withSecond(0).withNano(0), veryClose),
                  'Test 13: Failed to convert 24 hr time without seconds');
  testUtils.assert(time.toZDT('13:21:56').isClose(time.toZDT().withHour(13).withMinute(21).withSecond(56).withNano(0), veryClose),
                  'Test 13: Failed to convert 24 hr time with seconds');
  testUtils.assert(exceptionExpected(() => time.toZDT('25:00:00')),
                  'Test 13: Failed to throw exception for invalid time');

  // Test 14: 12 hr time
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT('1:21 pm').isClose(time.toZDT().withHour(13).withMinute(21).withSecond(0).withNano(0), veryClose),
                  'Test 14: Failed to convert 12 hr time without seconds');
  testUtils.assert(time.toZDT('01:21:56pm').isClose(time.toZDT().withHour(13).withMinute(21).withSecond(56).withNano(0), veryClose),
                  'Test 14: Failed to convert 12 hr time with seconds');
  testUtils.assert(exceptionExpected(() => time.toZDT('13:00:00 pm')),
                  'Test 14: Failed to throw exception for invalid time');

  // Test 15: Java ZonedDateTime.toString()
  console.info("Executing Test" + (++testNum));
  now = time.toZDT();
  javaNow = javaZDT.now();
  testUtils.assert(time.toZDT(javaNow.toString()).isClose(now, veryClose),
                  'Test 15: Failed to convert RFC string');
  
  // Test 16: Java ZonedDateTime.toString()
  console.info("Executing Test" + (++testNum));
  testUtils.assert(time.toZDT('PT10s').isClose(time.toZDT().plusSeconds(10), veryClose),
                  'Test 16: Duration String')

  // Test 17: Unsupported String
  console.info("Executing Test" + (++testNum));
  testUtils.assert(exceptionExpected(() => time.toZDT('unsupported')),
                   'Test 17: Failed to throw exception for unsupportable string');

  // Test 18: Number Item
  console.info("Executing Test" + (++testNum));
  var itemName = prefix+'Number';
  testOhType(itemName, 'Number', '1000');
  testUtils.assert(time.toZDT(items.getItem(itemName)).isClose(time.toZDT(1000), veryClose),
                  'Test 18: Failed to convert Number Item');
  testUtils.assert(time.toZDT(items.getItem(itemName).rawItem).isClose(time.toZDT(1000), veryClose),
                  'Test 18: Failed to convert raw Number Item');

  // Test 19: String Item
  console.info("Executing Test" + (++testNum));
  itemName = prefix+'String';
  testOhType(itemName, 'String', 'PT10S');
  testUtils.assert(time.toZDT(items.getItem(itemName)).isClose(time.toZDT(10000), veryClose),
                  'Test 19: Failed to convert String Item');
  testUtils.assert(time.toZDT(items.getItem(itemName).rawItem).isClose(time.toZDT(10000), veryClose),
                  'Test 19: Failed to convert raw String Item');

  // Test 20: DateTime Item
  console.info("Executing Test" + (++testNum));
  itemName = prefix+'DateTime';
  testOhType(itemName, 'DateTime', new DateTimeType().toString());
  testUtils.assert(time.toZDT(items.getItem(itemName)).isClose(time.toZDT(items.getItem(itemName).rawState), veryClose),
                  'Test 20: Failed to convert DateTimeType');
  testUtils.assert(time.toZDT(items.getItem(itemName).rawItem).isClose(time.toZDT(items.getItem(itemName).rawState), veryClose),
                  'Test 20: Failed to convert raw DateTimeType');

  // Test 21: Number:Time Item
  console.info("Executing Test" + (++testNum));
  itemName = prefix+'Quantity';
  testOhType(itemName, 'Number:Time', '10 s');
  var tdt = time.toZDT(items.getItem(itemName));
  now = time.toZDT('PT10s');
  testUtils.assert(time.toZDT(items.getItem(itemName)).isClose(time.toZDT('PT10s'), veryClose),
                  'Test 21: Failed to convert Number:Time');
  testUtils.assert(time.toZDT(items.getItem(itemName).rawItem).isClose(time.toZDT('PT10s'), veryClose),
                  'Test 21: Failed to convert raw Number:Time');

  // Test 22: Unsupported Item Type
  console.info("Executing Test" + (++testNum));
  itemName = prefix+'Switch';
  testOhType(itemName, 'Switch', 'ON');
  testUtils.assert(exceptionExpected(() => time.toZDT(items.getItem(itemName))),
                  'Test 22: Failed to throw exception for unsupported Item type');
  testUtils.assert(exceptionExpected(() => time.toZDT(items.getItem(itemName).rawItem)),
                  'Test 22: Failed to throw exception for unsupported raw Item type');

  // Test 23: toToday
  console.info("Executing Test" + (++testNum));
  var yesterday = time.toZDT().minusDays(1);
  testUtils.assert(time.toZDT().isClose(yesterday.toToday(), veryClose),
                   'Test 23: Failed to move yesterday to today');

  // Test 24: betweenTimes
  console.info("Executing Test" + (++testNum));
  now = time.toZDT();
  var before = now.withHour(20);
  var start = now.withHour(21);
  var between = now.withHour(22);
  var end = now.withHour(23);
  var after = now.withHour(3);
  testUtils.assert(!before.betweenTimes(start, end), 
                   'Test 24: Failed to find 20 outside between times');
  testUtils.assert(between.betweenTimes(start, end), 
                   'Test 24: Failed to find 22 inside between times');
  end = now.withHour(2);
  testUtils.assert(!before.betweenTimes(start, end), 
                   'Test 24: Failed spans midnight, before times');
  testUtils.assert(!after.betweenTimes(start, end), 
                   'Test 24: Failed spans midnight, after times');  
  testUtils.assert(between.betweenTimes(start, end), 
                   'Test 24: Failed spans midnight, after start before midnight');
  testUtils.assert(now.withHour(1).betweenTimes(start, end), 
                   'Test 24: Failed spans miodnight, before end after midnight');

}
catch(e) {
  console.error(`an error occured: ` + e);
}
finally {
/**
  Items apparently don't get registered, can't remove them
  testItems.forEach(i => {
    console.info('Deleting ' + i);
    items.removeItem(i);
  });
  */
}

console.log("toZDT tests are done");

The functions used from testUtils are:

const assert = (condition, message) => {
  if(!condition) {
    throw new Error(message || 'Assertion failed');
  }
}
  
const sleep = (msec) => {
  var curr = time.ZonedDateTime.now();
  var done = curr.plus(msec, time.ChronoUnit.MILLIS);
  var timeout = curr.plusSeconds(5);
  while (curr.isBefore(done) && curr.isBefore(timeout)) { // busy wait
    curr = time.ZonedDateTime.now();
  }
  //if(curr.isAfter(timeout)) console.error('sleep timed out!');
}

These are a part of openhab_rules_tools.

Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
@rkoshak
Copy link
Contributor Author

rkoshak commented Mar 1, 2022

Second attempt at this capability. I believe I've addressed all the comments from the previous PR and probably introduced all sorts of new ones since it's pretty much a complete rewrite.

I removed some functions as they probably are not that useful overall (toYesterday, toTomorrow) and which I no longer needed because I used a different implementation for betweenTimes. I added a new monkey patched isClose() method that tests the current ZDT and a passed in ZDT to see if they are within a time.Duration apart. I originally wrote it as part of my testing code but decided it's pretty useful in general.

@rkoshak
Copy link
Contributor Author

rkoshak commented Mar 15, 2022

I didn't say it which was a mistake.

This is ready for review.

README.md Outdated
Comment on lines 504 to 517
Argumen Type | Rule | Examples
-|-|-
`null` or `undefined` | `time.ZonedDateTime.now()` | `time.toZDT();`
`time.ZonedDateTime` | passed through unmodified |
`java.time.ZonedDateTime` | converted to the `time.ZonedDateTime` equivalent
JavaScript native `Date` | converted to the equivalent `time.ZonedDateTime` using `SYSTEM` as the timezone
`number`, `bingint`, `java.lang.Number`, `DecimalType` | rounded to the nearest integer and added to `now` as milliseconds | `time.toZDT(1000);`
`QuantityType` | if the units are `Time`, that time is added to `now` | `time.toZDT(item.getItem('MyTimeItem').rawState);`
`items.Item` or `org.openhab.core.types.Item` | if the state is supported (see the `Type` rules in this table, e.g. `DecimalType`), the state is converted | `time.toZDT(items.getItem('MyItem'));`
`String`, `java.lang.String`, `StringType` | parsed based on the following rules |
RFC String (output from a Java `ZonedDateTime.toString()`) | parsed | `time.toZDT(new DateTimeType().getZonedDateTime().toString());`
`"HH:MM[:ss]"` (24 hour time) | today's date with the time indicated, seconds is optional | `time.toZDT('13:45:12');`
`"kk:mm[:ss][ ]a"` (12 hour time) | today's date with the time indicated, the space between the time and am/pm and seconds are optional | `time.toZDT('1:23:45 PM');`
Duration String | any duration string supported by `time.Duration` added to `now()`, see [the docs](https://js-joda.github.io/js-joda/class/packages/core/src/Duration.js~Duration.html#static-method-parse) for details | `time.toZDT('PT1H4M6.789S');`
Copy link
Contributor

@digitaldan digitaldan Mar 19, 2022

Choose a reason for hiding this comment

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

Suggested change
Argumen Type | Rule | Examples
-|-|-
`null` or `undefined` | `time.ZonedDateTime.now()` | `time.toZDT();`
`time.ZonedDateTime` | passed through unmodified |
`java.time.ZonedDateTime` | converted to the `time.ZonedDateTime` equivalent
JavaScript native `Date` | converted to the equivalent `time.ZonedDateTime` using `SYSTEM` as the timezone
`number`, `bingint`, `java.lang.Number`, `DecimalType` | rounded to the nearest integer and added to `now` as milliseconds | `time.toZDT(1000);`
`QuantityType` | if the units are `Time`, that time is added to `now` | `time.toZDT(item.getItem('MyTimeItem').rawState);`
`items.Item` or `org.openhab.core.types.Item` | if the state is supported (see the `Type` rules in this table, e.g. `DecimalType`), the state is converted | `time.toZDT(items.getItem('MyItem'));`
`String`, `java.lang.String`, `StringType` | parsed based on the following rules |
RFC String (output from a Java `ZonedDateTime.toString()`) | parsed | `time.toZDT(new DateTimeType().getZonedDateTime().toString());`
`"HH:MM[:ss]"` (24 hour time) | today's date with the time indicated, seconds is optional | `time.toZDT('13:45:12');`
`"kk:mm[:ss][ ]a"` (12 hour time) | today's date with the time indicated, the space between the time and am/pm and seconds are optional | `time.toZDT('1:23:45 PM');`
Duration String | any duration string supported by `time.Duration` added to `now()`, see [the docs](https://js-joda.github.io/js-joda/class/packages/core/src/Duration.js~Duration.html#static-method-parse) for details | `time.toZDT('PT1H4M6.789S');`
| Argument Type | Rule | Examples |
|------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------|
| `null` or `undefined` | `time.ZonedDateTime.now()` | `time.toZDT();` |
| `time.ZonedDateTime` | passed through unmodified | |
| `java.time.ZonedDateTime` | converted to the `time.ZonedDateTime` equivalent | |
| JavaScript native `Date` | converted to the equivalent `time.ZonedDateTime` using `SYSTEM` as the timezone | |
| `number`, `bingint`, `java.lang.Number`, `DecimalType` | rounded to the nearest integer and added to `now` as milliseconds | `time.toZDT(1000);` |
| `QuantityType` | if the units are `Time`, that time is added to `now` | `time.toZDT(item.getItem('MyTimeItem').rawState);` |
| `items.Item` or `org.openhab.core.types.Item` | if the state is supported (see the `Type` rules in this table, e.g. `DecimalType`), the state is converted | `time.toZDT(items.getItem('MyItem'));` |
| `String`, `java.lang.String`, `StringType` | parsed based on the following rules | |
| RFC String (output from a Java `ZonedDateTime.toString()`) | parsed | `time.toZDT(new DateTimeType().getZonedDateTime().toString());` |
| `"HH:MM[:ss]"` (24 hour time) | today's date with the time indicated, seconds is optional | `time.toZDT('13:45:12');` |
| `"kk:mm[:ss][ ]a"` (12 hour time) | today's date with the time indicated, the space between the time and am/pm and seconds are optional | `time.toZDT('1:23:45 PM');` |
| Duration String | any duration string supported by `time.Duration` added to `now()`, see [the docs](https://js-joda.github.io/js-joda/class/packages/core/src/Duration.js~Duration.html#static-method-parse) for details | `time.toZDT('PT1H4M6.789S');` |

FYI , I use http://markdowntable.com to format tables like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix it. Thanks for the link to markdowntable.com. That's a great resource!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@digitaldan digitaldan left a comment

Choose a reason for hiding this comment

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

Thanks @rkoshak ! See comments.

* @returns time.ZonedDateTime
* @throws error if the type, format, or contents of when are not supported
*/
time.toZDT = function(when) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a really easy and awesome method to have some tests around, so pass in each of the listed dates in the comment and verify you get the right zdt time back.

Copy link
Contributor

Choose a reason for hiding this comment

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

lol, i just re-read your original post and saw you had a lot of testing! Can you add those tests to this library ? There is a test directory with some tests around the rule builder, but should provide a good template for adding the testing logic you have already written for the time classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me how tests are handled in the library. I've just got my tests as a Script in MainUI and I use a couple of functions from my openhab-rules-tools library to do 'assert` and such. I'll look more into how the rule builder tests work and see if I can figure out how to convert to that format.

I always write tests as I go. ;-) I'm more than happy to get them checked in to the repo. (As an aside I've tests for toggle too which I'll see if I can get added to that PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still trying to figure out how to make Mocha work. I've installed the libraries as global but they are not showing up. In short I've not even managed to run the tests that are already there. I'm still muddling through but I've held this PR up long enough already. Maybe it's worth coming back later with a new PR.

time.js Outdated
}

// Pass through if already a time.ZonedDateTime
else if(when instanceof time.ZonedDateTime) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else if(when instanceof time.ZonedDateTime) {
if(when instanceof time.ZonedDateTime) {

since each of these if statements return if matched, there no need for else in any of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure. I've used StandardJS as a style guide but it seemed to be silent on this topic. I don't have an opinion either way so have no problem removing the unnecessary else statements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

time.js Outdated
}

/**
* Tests to see if the difference betwee this and the passed in ZoneDateTime is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Tests to see if the difference betwee this and the passed in ZoneDateTime is
* Tests to see if the difference between this and the passed in ZoneDateTime is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

time.js Outdated
/**
* Tests to see if the difference betwee this and the passed in ZoneDateTime is
* within the passed in maxDur.
* @param {time.ZonedDateTime} zdt the date tiem to compare to this
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {time.ZonedDateTime} zdt the date tiem to compare to this
* @param {time.ZonedDateTime} zdt the date time to compare to this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

time.js Outdated
time.ZonedDateTime.prototype.millisFromNow = function () {
return time.Duration.between(time.ZonedDateTime.now(), this).toMillis();
};
time.ZonedDateTime.prototype.betweenTimes = function(start, end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
time.ZonedDateTime.prototype.betweenTimes = function(start, end) {
time.ZonedDateTime.prototype.isBetweenTimes = function(start, end) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* @param {time.Duration} maxDur the duration to test that the difference between this and zdt is within
* @returns {Boolean} true if the delta between this and zdt is within maxDur
*/
time.ZonedDateTime.prototype.isClose = function(zdt, maxDur) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm struggling with this method a bit, it seems pretty hard to explain (maybe is needs a better name and better named parameters?) , and also seems very niche in its use case. I'm wondering how frequently something like this will be used by others? I think we should strive to only add in whats really going to be useful to a broader audience, otherwise our library will start to seem inconsistent and unfocused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen a number of users struggle with the fact that you'll probably never end up with two timestamps that are actually equal. They are surprised that something like time.ZonedDateTime.now() == time.ZonedDateTime.now() will be false. This was. an attempt to give them a function I could point them to to help them make such similar comparisons as well as kind of explain what's going on.

It's also handy in cases where you want to see how far apart two time stamps are (for example, if a motion detector triggered within five minutes of a light switch flipping).

I use the heck out of this function in the tests and originally it was only in my test code. But I ran into a few use cases where it seemed useful in general. I can definitely move it to the tests, though will probably also reimplement it in my openhab-rules-tools since I've already two rules I've identified that can benefit from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't removed this function. I personally find it useful. But if you feel strongly about it I will remove it.

time.js Outdated
}

// ISO8601
else if(isISO8601(str)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else if(isISO8601(str)) {
if(isISO8601(str)) {

since each of these if statements return if matched, there no need for else in any of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
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.

Overall looks really good, please have a look at my comments.

I have not tested the changes, but if @digitaldan wants me to do so, I can do.

Anywhere that a native Java `ZonedDateTime` or `Duration` is required, the runtime will automatically convert a JS-Joda `ZonedDateTime` or `Duration` to its Java counterpart.

#### openHAB-JS extensions to JS-Joda
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can keep that overview of our additions/changes to JSJoda and you can add your changes from this PR here?

Copy link
Contributor Author

@rkoshak rkoshak May 2, 2022

Choose a reason for hiding this comment

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

I don't have a problem with that except much of what I've implemented is outside of JS-Joda and not extensions to it. So that heading as written is incorrect, or at least there would need to be two sections, one for the additions to JS-Joda and another one for toZDT().

But my branch doesn't have the millisFromNow. I suspect that was added after I opened this PR. I removed that heading because all that was under it was "Examples:" on down. There wasn't anything there to preserve and the bulk of the content was no longer references to JS-Joda.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, leave it as it is.

I will have a look what happens when this gets merged. If millisFromNow is removed, I will add it back.

Examples:
```javascript
var now = time.ZonedDateTime.now();
var yesterday = time.ZonedDateTime.now().minusHours(24);
var millis = now.plusSeconds(5).millisFromNow();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not remove this!

Copy link
Contributor Author

@rkoshak rkoshak May 2, 2022

Choose a reason for hiding this comment

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

I didn't. It was never there in the first place. See below, it wasn't supposed to be there and I accidentally pulled it in when I erroneously rebaselined my branch and was asked to back those out (see above).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, leave it as it is.


var item = items.getItem("Kitchen");
console.log("averageSince", item.history.averageSince(yesterday));
console.log("5 seconds in the future is " + millis + " milliseconds.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not remove!

Copy link
Contributor Author

@rkoshak rkoshak May 2, 2022

Choose a reason for hiding this comment

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

I didn't, it wasn't supposed to be there in the first place. I didn't remove anything from the README except that heading. Everything else was an addition. Though I had inadvertently rebaselined my branch and those were probably pulled in when I made that mistake. Dan asked me to back those out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, leave it as it is.

README.md Outdated
Examples:

```javascript
time.toZDT().beteenTimes('22:00', '05:00') // currently between 11:00 pm and 5:00 am
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
time.toZDT().beteenTimes('22:00', '05:00') // currently between 11:00 pm and 5:00 am
time.toZDT().betweenTimes('22:00', '05:00') // currently between 11:00 pm and 5:00 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the next commit.

README.md Outdated
```javascript
time.toZDT().beteenTimes('22:00', '05:00') // currently between 11:00 pm and 5:00 am
time.toZDT().betweenTimes(items.getItem('Sunset'), '11:30 PM') // is now between sunset and 11:30 PM?
time.toZDT(items.getItem('StartTime')).bewteenTimes(time.toZDT(), 'PT1H'); // is the state of StartTime between now and one hour from now
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
time.toZDT(items.getItem('StartTime')).bewteenTimes(time.toZDT(), 'PT1H'); // is the state of StartTime between now and one hour from now
time.toZDT(items.getItem('StartTime')).betweenTimes(time.toZDT(), 'PT1H'); // is the state of StartTime between now and one hour from now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the next commit

*
* @memberof time
* @returns {number} duration from now to the ZonedDateTime in milliseconds
* Moves the date portion of the date time to today, accounting for DST
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the @memberof time tag for JSDoc here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a monkey patch of ZonedDateTime. If it's a member of anything it's a member of time.ZonedDateTime, not time. This @memberof time tag makes absolutely no sense to me.

Change made in next commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above, you can remove @memberof time.

time.js Outdated
*
* time.ZonedDateTime.now().betweenTimes(items.getItem('Sunset'), "23:30:00") // is now between sunset and 11:00 pm?
* time.toZDT(items.geItem('MyDateTimeItem')).betweenTimes(time.toZDT(), time.toZDT(1000)) // is the state of MyDateTimeItem between now and a second from now?
*
Copy link
Contributor

Choose a reason for hiding this comment

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

We need @memberof time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made in next commit.

time.js Outdated
* @returns {Boolean} true if this is between start and end
*/
time.ZonedDateTime.prototype.isBetweenTimes = function(start, end) {
startTime = time.toZDT(start).toLocalTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
startTime = time.toZDT(start).toLocalTime();
const startTime = time.toZDT(start).toLocalTime();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit.

time.js Outdated
*/
time.ZonedDateTime.prototype.isBetweenTimes = function(start, end) {
startTime = time.toZDT(start).toLocalTime();
endTime = time.toZDT(end).toLocalTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
endTime = time.toZDT(end).toLocalTime();
const endTime = time.toZDT(end).toLocalTime();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next commit

time.js Outdated
time.ZonedDateTime.prototype.isBetweenTimes = function(start, end) {
startTime = time.toZDT(start).toLocalTime();
endTime = time.toZDT(end).toLocalTime();
currTime = this.toLocalTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
currTime = this.toLocalTime();
const currTime = this.toLocalTime();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next commit

Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
@rkoshak
Copy link
Contributor Author

rkoshak commented May 2, 2022

The requested changes have been made except for adding back in stuff that shouldn't have been in this PR in the first place.

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.

Looks good to me, thank you very much.

I am sorry for the wrong @memberof time tags, you can remove them if you wish to, except for toZDT, toToday, isBetweenTimes, isClose. Right??

* Adds millis to the passed in ZDT as milliseconds. The millis is rounded
* first. If millis is negative they will be subtracted.
* @param {number|bigint} millis number of milliseconds to add
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I just checked, it is not exported.
You are totally right, @memberof time makes no sense here. I am not sure why I wrote that yesterday, must have been a misthought.

/**
* Converts the passed in when to a time.ZonedDateTime based on the following
* set of rules.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, it only needs @private but does not need @memberof time.

*/
time.ZonedDateTime.prototype.millisFromNow = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, then leave it as it is.

*
* @memberof time
* @returns {number} duration from now to the ZonedDateTime in milliseconds
* Moves the date portion of the date time to today, accounting for DST
Copy link
Contributor

Choose a reason for hiding this comment

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

See above, you can remove @memberof time.

Signed-off-by: Richard Koshak <rlkoshak@gmail.com>
@rkoshak
Copy link
Contributor Author

rkoshak commented May 2, 2022

I'm still confused about why it's @memberof time as opposed to @memberof time.ZonedDateTime for the monkey patch functions. I've left them as just @memberof time and removed the tag from the functions that are not exposed.

@florian-h05
Copy link
Contributor

as opposed to @memberof time.ZonedDateTime for the monkey patch functions

Yes, that would make more sense, but I do not know if that works with the JSDoc htmls generated. You can give it a try and run npm run docs and then have a look at the generated htmls. I think you need to define a new namespace time.ZonedDateTime.

@rkoshak
Copy link
Contributor Author

rkoshak commented May 3, 2022

Interestingly, neither time nor time.ZonedDateTime generates anything at all for the monkey patched methods. I'm not sure what to do. Since neither work it seems like I should remove them and we'll just have the docs added to the README.MD for the new functions.

@florian-h05
Copy link
Contributor

Since neither work it seems like I should remove them and we'll just have the docs added to the README.MD for the new functions.

I would‘t remove them as they are at least helpful when scrolling through the source code, but yes, the README seems to be the only place where the monkey-patched functions can be documented (outside the source code).

@rkoshak
Copy link
Contributor Author

rkoshak commented May 3, 2022

In that case I'd say let's just leave them as is and consider this PR good to go.

@digitaldan
Copy link
Contributor

Some conflicts popped up from an earlier PR that was merged, can you fix those? Thanks!

@rkoshak
Copy link
Contributor Author

rkoshak commented May 9, 2022

Fixing the conflict was easy for README.MD but it's marking everything I've added to time.js as a conflict. How to I fix this without basically reversing everything I've added? I don't have write access so I can't tell it to use my version instead of main. I'm not sure what to do.

@digitaldan
Copy link
Contributor

I don't have write access so I can't tell it to use my version instead of main. I'm not sure what to do.

Your PR is all that needs to be changed. Fetch the main branch, then in your PR branch do a git merge main or you can use git rebase main . This will result in conflicts and give instructions on how to proceed. From here you remove the lines in the file which show the conflicts like <<<<<<< toZDT , this should be simple as the conflicts are all new inserts, nothing is actually being changed from main (its odd git didn't handle this better), then do a git add time.js and do a commit and push.

@digitaldan digitaldan merged commit 642fcc3 into openhab:main May 13, 2022
@digitaldan
Copy link
Contributor

I went ahead and fixed the conflicts , so all is merged now.

@rkoshak
Copy link
Contributor Author

rkoshak commented May 16, 2022

Thanks! I thought I wasn't supposed to fetch from main because last time I did that in the toggle PR I ended up with someone else's PR mixed in and it appears to have caused all sorts of problems. Sometimes I miss the simplicity of good old CVS. I feel like a bull in a china shop every time I mess with a PR on Github.

The good news is I think I've figured out Mocha so I'll be able to add some unit tests in a separate PR when I can implement them. I'll look into what it would take to create tests for as much in the library as we can.

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

Successfully merging this pull request may close these issues.

3 participants