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

[enturno] Remove org.apache.commons #14406

Merged
merged 14 commits into from
Oct 27, 2023
Merged

[enturno] Remove org.apache.commons #14406

merged 14 commits into from
Oct 27, 2023

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Feb 16, 2023

  • Remove dependency on org.apache.commons
  • Moved model to dto to fix warnings
  • Extracted, improved and created tests for getIsoDateTime (thanks @jlaur)

Untested, minor refactoring from mainly from code perspective

Updated test jar 4.1.0: https://1drv.ms/u/s!AnMcxmvEeupwjq5aI8LUb81H1d5OdQ?e=FhltuE

Signed-off-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Apr 7, 2023

@klocsson are you able to review?

@jlaur jlaur added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label May 9, 2023
Signed-off-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel lsiepel requested a review from jlaur October 20, 2023 17:26
@jlaur
Copy link
Contributor

jlaur commented Oct 21, 2023

@lsiepel - I'm wondering if you could be persuaded into writing a few unit tests (positive and negative)? 🙂 Since this is not manually tested it would be a quite convincing proof that it works correctly.

It's not a lot of code, but for me to assess if it actually works as intended for some different scenarios, I think writing tests would almost be faster than to manually go through the code case by case with different inputs, read the implementations of the methods called before/after the change and calculate the outputs from that.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 26, 2023

@lsiepel - I'm wondering if you could be persuaded into writing a few unit tests (positive and negative)? 🙂 Since this is not manually tested it would be a quite convincing proof that it works correctly.

It's not a lot of code, but for me to assess if it actually works as intended for some different scenarios, I think writing tests would almost be faster than to manually go through the code case by case with different inputs, read the implementations of the methods called before/after the change and calculate the outputs from that.

Just added some tests. I must say that the implementation of getIsoDateTime is not very good. But improving that method is out of scope if this PR. I want to stay as close to the current implementation as possible as that is an accepted version.

The method:

  • does not check input strings , so mallformed inputs might causes unwanted behaviour
  • only seems to accept zones ahead of UTC (only checking for + and not - sign)
  • only accepts zones with full hour, not the 30/45 minute in between zones.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@jlaur
Copy link
Contributor

jlaur commented Oct 26, 2023

Just added some tests. I must say that the implementation of getIsoDateTime is not very good. But improving that method is out of scope if this PR. I want to stay as close to the current implementation as possible as that is an accepted version.

The method:

  • does not check input strings , so mallformed inputs might causes unwanted behaviour
  • only seems to accept zones ahead of UTC (only checking for + and not - sign)
  • only accepts zones with full hour, not the 30/45 minute in between zones.

Seeing the tests and your comment made me realize the actual goal here (I think), so I have given it some attention due to a strange obsession of mine with date and time logic. 😄 First thing I did was running your tests on the original method:

[ERROR] Failures: 
[ERROR]   EnturNoConnectionTest.getIsoDateTime_WithoutZone:41 expected: <2023-10-25T09:01:00+00:00> but was: <2023-10-25T09:01:00+:00>

So it seems that we don't actually need to handle the case of missing offset because it was never supported and the parameter name dateTimeWithoutColonInZone actually suggests that the only issue is a missing colon in the offset.

I tried to create a PR towards your branch, but realized I would either need to have access to your repository, fork it or be a Git expert. Anyway, I have some proposed code here. There's nothing wrong with yours, I just wanted to give it a shot to further improve.

New class:

/**
 * Copyright (c) 2010-2023 Contributors to the openHAB project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */
package org.openhab.binding.enturno.internal.util;

import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;

import org.eclipse.jdt.annotation.NonNullByDefault;

/**
 * EnturNo date utility methods.
 *
 * @author Jacob Laursen - Initial contribution
 */
@NonNullByDefault
public class DateUtil {
    /**
     * Converts a zoned date time string that lacks a colon in the zone to an ISO-8601 formatted string.
     * 
     * @param dateTimeWithoutColonInZone
     * @return ISO-8601 formatted string
     */
    public static String getIsoDateTime(String dateTimeWithoutColonInZone) {
        ZonedDateTime zonedDateTime = null;
        try {
            zonedDateTime = ZonedDateTime.parse(dateTimeWithoutColonInZone);
        } catch (DateTimeParseException e) {
            // Skip
        }
        try {
            zonedDateTime = ZonedDateTime.parse(dateTimeWithoutColonInZone.replaceAll("(\\d{2})(\\d{2})$", "$1:$2"));
        } catch (DateTimeParseException e) {
            // Skip
        }
        if (zonedDateTime != null) {
            return DateTimeFormatter.ISO_OFFSET_DATE_TIME.format(zonedDateTime);
        }
        return dateTimeWithoutColonInZone;
    }
}

and corresponding test class:

/**
 * Copyright (c) 2010-2023 Contributors to the openHAB project
 *
 * See the NOTICE file(s) distributed with this work for additional
 * information.
 *
 * This program and the accompanying materials are made available under the
 * terms of the Eclipse Public License 2.0 which is available at
 * http://www.eclipse.org/legal/epl-2.0
 *
 * SPDX-License-Identifier: EPL-2.0
 */
package org.openhab.binding.enturno.internal.util;

import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.MatcherAssert.assertThat;

import java.util.stream.Stream;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

/**
 * Tests for {@link DateUtil}.
 *
 * @author Jacob Laursen - Initial contribution
 */
@NonNullByDefault
public class DateUtilTest {
    @ParameterizedTest
    @MethodSource("provideTestCasesForGetIsoDateTime")
    void getIsoDateTime(String value, String expected) {
        assertThat(DateUtil.getIsoDateTime(value), is(expected));
    }

    private static Stream<Arguments> provideTestCasesForGetIsoDateTime() {
        return Stream.of( //
                Arguments.of("2023-10-25T09:01:00+0200", "2023-10-25T09:01:00+02:00"),
                Arguments.of("2023-10-25T09:01:00+02:00", "2023-10-25T09:01:00+02:00"),
                Arguments.of("2023-10-25T09:01:00-0300", "2023-10-25T09:01:00-03:00"),
                Arguments.of("2023-10-25T09:01:00+02:30", "2023-10-25T09:01:00+02:30"),
                Arguments.of("2023-10-25T09:01:00", "2023-10-25T09:01:00"));
    }
}

Some notes:

  • I kept only the two relevant formats, i.e. standard format and the format without colon in offset.
  • I extracted the method into a separate class because it was already made static and public, so didn't have a strong relationship.
  • I refactored the unit tests into contraint-based assert model (assertThat) and parameterized the test cases to improve readability and reduce boiler-plate code.

Let me know what you think. With the added test coverage we should have minimized the risk and have also proven that the previous cases are still working (with the addition of further cases).

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 27, 2023

Let me know what you think. With the added test coverage we should have minimized the risk and have also proven that the previous cases are still working (with the addition of further cases).

Allways a bit balancing between the goal of the PR and the issues that i discover in the proces. I also thought about extracting the method and improve it. But now i actually did with your code snippet.

The method can be further improved, but as the current implementation allready handles much more cases (all time zones etc) then before i think it is good as is. Thanks, the regex is something i didn't think about.

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

@jlaur jlaur merged commit ab5ebbc into openhab:main Oct 27, 2023
3 checks passed
@jlaur jlaur added this to the 4.1 milestone Oct 27, 2023
@lsiepel lsiepel deleted the enturno branch October 28, 2023 06:31
truidix pushed a commit to truidix/openhab-addons that referenced this pull request Oct 30, 2023
* Remove org.apache.commons
* Fix warnings

Also-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* Remove org.apache.commons
* Fix warnings

Also-by: Jacob Laursen <jacob-github@vindvejr.dk>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
additional testing preferred The change works for the pull request author. A test from someone else is preferred though.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants