[core] Added new methods toZone and toLocaleZone to DateTimeType #945

lolodomo · 2019-07-27T16:56:48Z

Enhanced tests

Signed-off-by: Laurent Garnier [email protected]

lolodomo · 2019-07-27T17:07:52Z

@cweitkamp : here are the additional format methods with tests. This works well and as expected.

Now, we have the choice to link the old format methods with the new format methods using ZoneId.systemDefault() as zone. This will impact all calls to the old format methods. Impact not yet evaluated.
Or we keep unchanged the existing format methods that will continue ignoring the system timezone for formatting dates and I just update the UI code to use the new format methods with ZoneId.systemDefault() as parameter to get dates in system timezone in UI.

cweitkamp

Thank you.

I do not want to reject your new methods to format the value but I am still thinking a toZone(ZoneId/String) method (similar to the toUnit() method from QuantityType) will be more useful.

link the old format methods with the new format methods

No, the framework should not do that (see #832 (comment)). Implementing it in Basic UI will be okay. Did you check how it is done in HABmin?

cweitkamp · 2019-07-28T07:14:35Z

...rg.openhab.core/src/test/java/org/eclipse/smarthome/core/library/types/DateTimeTypeTest.java


        DateTimeType dt2 = new DateTimeType(dt1.getZonedDateTime().withZoneSameInstant(ZoneId.of("Europe/Berlin")));
        assertThat(dt2, not(is(dt1)));
        assertThat(dt2.toString(), is("2019-05-25T19:53:10.000+0200"));

        assertThat(dt2.format(null), is("2019-05-25T19:53:10"));
        assertThat(dt2.format("%1$td.%1$tm.%1$tY %1$tH:%1$tM"), is("25.05.2019 19:53"));
-
-        assertThat(dt2.format(Locale.GERMAN, "%1$td.%1$tm.%1$tY %1$tH:%1$tM"), is("25.05.2019 19:53"));
+        assertThat(dt1.format(null, gmt), is("2019-05-25T17:53:10"));


Should be dt2 in this and the following lines.

lolodomo · 2019-07-28T11:15:16Z

I am going to enhance the tests.

Ok for the idea of the toZone method rather than my new format methods.

Enhanced tests Signed-off-by: Laurent Garnier <[email protected]>

lolodomo · 2019-07-29T10:55:52Z

@cweitkamp : PR is now finished and ready for a review.

I will change the REST sitemap by using the new methods in a following PR.

cweitkamp · 2019-07-29T12:05:29Z

...es/org.openhab.core/src/main/java/org/eclipse/smarthome/core/library/types/DateTimeType.java

+    /**
+     * Translate to a given zone
+     *
+     * @param zone the destination zone


Suggested change

* @param zone the destination zone

* @param zone the target {@link ZoneId}

I am not sure if ZoneId is available in DSL rules or NGRE thus I suggest to add a toUnit(String zone) method too.

Ok I will add it.

cweitkamp · 2019-07-29T19:25:01Z

...es/org.openhab.core/src/main/java/org/eclipse/smarthome/core/library/types/DateTimeType.java

@@ -108,7 +109,7 @@ public DateTimeType(String zonedValue) {
            throw new IllegalArgumentException(zonedValue + " is not in a valid format.", invalidFormatException);
        }

-        zonedDateTime = date;
+        zonedDateTime = date.withFixedOffsetZone();


It looks like the right thing to streamline the API. But in opposition I found eclipse-archived/smarthome#5076 which questions if withFixedOffsetZone() is the right way to go. Unfortunately with no result. @openhab/core-maintainers wdyt?

withFixedOffsetZone() is applied to the other constructors so it makes sense to apply to this one too. It allows to get identical DateTimeType when calling the different constructors with the same data.

cweitkamp · 2019-07-30T06:19:31Z

...es/org.openhab.core/src/main/java/org/eclipse/smarthome/core/library/types/DateTimeType.java

+     * @param zone the destination zone
+     */
+    public void toZone(ZoneId zone) {
+        zonedDateTime = zonedDateTime.withZoneSameInstant(zone);


You might consider to return a new copy of DateTimeType here instead (like

openhab-core/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/library/types/QuantityType.java

Lines 187 to 207 in 9e7c6af

/**

* Convert this QuantityType to a new {@link QuantityType} using the given target unit.

*

* @param targetUnit the unit to which this {@link QuantityType} will be converted to.

* @return the new {@link QuantityType} in the given {@link Unit} or {@code null} in case of a

*/

@SuppressWarnings("unchecked")

public @Nullable QuantityType<T> toUnit(Unit<?> targetUnit) {

if (!targetUnit.equals(getUnit())) {

try {

UnitConverter uc = getUnit().getConverterToAny(targetUnit);

Quantity<?> result = Quantities.getQuantity(uc.convert(quantity.getValue()), targetUnit);

return new QuantityType<T>(result.getValue(), (Unit<T>) targetUnit);

} catch (UnconvertibleException | IncommensurableException e) {

logger.debug("Unable to convert unit from {} to {}", getUnit(), targetUnit);

return null;

}

}

return this;

}

)

lolodomo · 2019-07-30T06:33:34Z

Ok. I hesitated between the two options.

Signed-off-by: Laurent Garnier <[email protected]>

lolodomo · 2019-07-30T22:28:07Z

@cweitkamp : changes done.
Please let me know if you prefer that the methods throw an exception or return null in case of problem.

cweitkamp

As both exceptions are inherited of RuntimeException and they will be thrown if the user passes some wrong values it would be okay for me if we do not catch them anywhere.

cweitkamp · 2019-08-01T07:06:21Z

...rg.openhab.core/src/test/java/org/eclipse/smarthome/core/library/types/DateTimeTypeTest.java

+        boolean thrown = false;
+        try {
+            dt2 = dt.toZone("XXX");
+        } catch (DateTimeException e) {


This should be an own test which expects the thrown exception:

@Test(expected = DateTimeException.class) public void changingZoneThrowsExceptionTest() { // TODO }

cweitkamp · 2019-08-01T07:09:08Z

...rg.openhab.core/src/test/java/org/eclipse/smarthome/core/library/types/DateTimeTypeTest.java

+         * @param pattern
+         * @param expectedFormattedResult
+         */
+        public ParameterSet(TimeZone defaultTimeZone, Map<String, Integer> inputTimeMap, TimeZone inputTimeZone,


I am just wondering if we can reuse this constructor called by the other constructors. But that is only a side comment. Up to you.

Yes we can.

...rg.openhab.core/src/test/java/org/eclipse/smarthome/core/library/types/DateTimeTypeTest.java

lolodomo · 2019-08-01T17:04:00Z

Do you know if this is possible to run a particular test only once and not for each ParsmeterSet?

Signed-off-by: Laurent Garnier <[email protected]>

cweitkamp · 2019-08-01T21:04:41Z

No, I am afraid, I do not know. But does it hurt?

lolodomo · 2019-08-01T21:07:07Z

No, it just certainly takes few additional ms to run.

cweitkamp

Thanks.

@openhab/core-maintainers Any objections regarding the withFixedOffsetZone() method?

kaikreuzer · 2019-08-02T10:57:24Z

@cweitkamp I am honestly not really clear on the effect of that change, but as you have looked deeper into it and approved it, I don't see any reason not to merge it :-)

lolodomo · 2019-08-02T11:00:36Z

Cool, I can move to the next step, using these methods to fix a bug.

cweitkamp · 2019-08-02T11:11:29Z

Just for clarification: From https://docs.oracle.com/javase/8/docs/api/java/time/ZonedDateTime.html#withFixedOffsetZone--

This returns a zoned date-time where the zone ID is the same as getOffset(). The local date-time, offset and instant of the result will be the same as in this date-time.

Setting the date-time to a fixed single offset means that any future calculations, such as addition or subtraction, have no complex edge cases due to time-zone rules. This might also be useful when sending a zoned date-time across a network, as most protocols, such as ISO-8601, only handle offsets, and not region-based zone IDs.

Which basically means that it cuts off the "US/Central" (or whatever timezone you are in) string from the object (you can see that in the debugger). The quoted text above also states that cutting it off simplifies calculations based on this object later on (e.g. 2018-11-04T01:25:43-06:00[US/Central] results in 2018-11-04T01:25:43-06:00).

* Added some explanation for the website build. * Adapted to jenkins config changes. * Removed additional blank line. Signed-off-by: Jerome Luckenbach <[email protected]>

Signed-off-by: Laurent Garnier <[email protected]> GitOrigin-RevId: f03f328

cweitkamp reviewed Jul 28, 2019

View reviewed changes

lolodomo changed the title ~~[WIP] DateTimeType: format using a provided zone~~ [WIP] DateTimeType: new methods toZone and toLocaleZone Jul 28, 2019

DateTimeType: new methods toZone and toLocaleZone

1fef478

Enhanced tests Signed-off-by: Laurent Garnier <[email protected]>

lolodomo changed the title ~~[WIP] DateTimeType: new methods toZone and toLocaleZone~~ DateTimeType: new methods toZone and toLocaleZone Jul 29, 2019

cweitkamp reviewed Jul 29, 2019

View reviewed changes

cweitkamp reviewed Jul 30, 2019

View reviewed changes

Methods toZone and toLocaleZone returning a DateTimeType

85a2935

Signed-off-by: Laurent Garnier <[email protected]>

cweitkamp reviewed Aug 1, 2019

View reviewed changes

Tests updated

38083b8

Signed-off-by: Laurent Garnier <[email protected]>

cweitkamp added rebuild Triggers the Jenkins PR build and removed rebuild Triggers the Jenkins PR build labels Aug 1, 2019

5iver mentioned this pull request Aug 1, 2019

Fix date module openhab-scripters/openhab-helper-libraries#193

Merged

cweitkamp approved these changes Aug 2, 2019

View reviewed changes

kaikreuzer merged commit f03f328 into openhab:master Aug 2, 2019

lolodomo deleted the DateTimeType branch August 2, 2019 10:58

cweitkamp added this to the 2.5 milestone Aug 3, 2019

cweitkamp added the enhancement An enhancement or new feature of the Core label Aug 3, 2019

cweitkamp changed the title ~~DateTimeType: new methods toZone and toLocaleZone~~ [core] Added new methods toZone and toLocaleZone to DateTimeType Dec 3, 2019

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023

DateTimeType: new methods toZone and toLocaleZone (openhab#945)

ef5dc89

Signed-off-by: Laurent Garnier <[email protected]> GitOrigin-RevId: f03f328

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Added new methods toZone and toLocaleZone to DateTimeType #945

[core] Added new methods toZone and toLocaleZone to DateTimeType #945

lolodomo commented Jul 27, 2019 •

edited

Loading

lolodomo commented Jul 27, 2019 •

edited

Loading

cweitkamp left a comment

cweitkamp Jul 28, 2019

lolodomo commented Jul 28, 2019

lolodomo commented Jul 29, 2019

cweitkamp Jul 29, 2019

lolodomo Jul 30, 2019

cweitkamp Jul 29, 2019 •

edited

Loading

lolodomo Jul 30, 2019

cweitkamp Jul 30, 2019 •

edited

Loading

lolodomo commented Jul 30, 2019

lolodomo commented Jul 30, 2019

cweitkamp left a comment

cweitkamp Aug 1, 2019

lolodomo Aug 1, 2019

cweitkamp Aug 1, 2019

lolodomo Aug 1, 2019

lolodomo Aug 1, 2019

lolodomo commented Aug 1, 2019

cweitkamp commented Aug 1, 2019

lolodomo commented Aug 1, 2019

cweitkamp left a comment

kaikreuzer commented Aug 2, 2019

lolodomo commented Aug 2, 2019

cweitkamp commented Aug 2, 2019

	* @param zone the destination zone
	* @param zone the target {@link ZoneId}

	/**
	* Convert this QuantityType to a new {@link QuantityType} using the given target unit.
	*
	* @param targetUnit the unit to which this {@link QuantityType} will be converted to.
	* @return the new {@link QuantityType} in the given {@link Unit} or {@code null} in case of a
	*/
	@SuppressWarnings("unchecked")
	public @Nullable QuantityType<T> toUnit(Unit<?> targetUnit) {
	if (!targetUnit.equals(getUnit())) {
	try {
	UnitConverter uc = getUnit().getConverterToAny(targetUnit);
	Quantity<?> result = Quantities.getQuantity(uc.convert(quantity.getValue()), targetUnit);

	return new QuantityType<T>(result.getValue(), (Unit<T>) targetUnit);
	} catch (UnconvertibleException \| IncommensurableException e) {
	logger.debug("Unable to convert unit from {} to {}", getUnit(), targetUnit);
	return null;
	}
	}
	return this;
	}

[core] Added new methods toZone and toLocaleZone to DateTimeType #945

[core] Added new methods toZone and toLocaleZone to DateTimeType #945

Conversation

lolodomo commented Jul 27, 2019 • edited Loading

lolodomo commented Jul 27, 2019 • edited Loading

cweitkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Jul 28, 2019

lolodomo commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweitkamp Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

lolodomo commented Jul 30, 2019

lolodomo commented Jul 30, 2019

cweitkamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolodomo commented Aug 1, 2019

cweitkamp commented Aug 1, 2019

lolodomo commented Aug 1, 2019

cweitkamp left a comment

Choose a reason for hiding this comment

kaikreuzer commented Aug 2, 2019

lolodomo commented Aug 2, 2019

cweitkamp commented Aug 2, 2019

lolodomo commented Jul 27, 2019 •

edited

Loading

lolodomo commented Jul 27, 2019 •

edited

Loading

cweitkamp Jul 29, 2019 •

edited

Loading

cweitkamp Jul 30, 2019 •

edited

Loading