openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

[items] ItemHistory: Change return types of min/max between/since to number #175

Closed florian-h05 closed 1 year ago

florian-h05 commented 1 year ago

Changes the return types of ItemHistory.

from string to number.

Possibly breaking.

florian-h05 commented 1 year ago

@jpg0 Can you review?

jpg0 commented 1 year ago

@jpg0 Can you review?

Could you add into the function descriptions what the units of then returned numbers are? I would hope/expect they match the units of the parameters.

florian-h05 commented 1 year ago

I would hope/expect they match the units of the parameters.

Not sure what you mean with this.

what the units of then returned numbers are?

The units would be could of, but I think that the current code wouldn’t even work. I’ll test this later.

I am wondering if we shouldn’t introduce a library for UoM handling like https://github.com/gentooboontoo/js-quantities? WDYT?

jpg0 commented 1 year ago

Personally I don't believe that we should introduce a units library, at least not yet.

I just wanted to ensure that the return units are easily handled in JS - for example they are milliseconds. I'm not sure what they are from the code/docs.

florian-h05 commented 1 year ago

I just wanted to ensure that the return units are easily handled in JS - for example they are milliseconds. I'm not sure what they are from the code/docs.

It seems that:

But I'm not sure.

florian-h05 commented 1 year ago

@jpg0 Are you fine with my little addition to the docs regarding the units?

florian-h05 commented 1 year ago

@jpg0 I've just noticed that either number or NaN is returned, so either I correct the JSDoc or I modify the function to return number or null as all other functions do.

What would you prefer?

jpg0 commented 1 year ago

If the other functions return null, then let's keep with that.