Skip to content

switch from joda.time to java.time #1746

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
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

switch from joda.time to java.time #1746

wants to merge 1 commit into from
+489 −509

Conversation

juherr
Copy link
Contributor

@juherr juherr commented Apr 2, 2025

Close #98

@juherr
Copy link
Contributor Author

juherr commented Apr 2, 2025

Depends on steve-community/ocpp-jaxb#16

@juherr juherr marked this pull request as draft April 2, 2025 16:45
juherr
juherr commented Apr 2, 2025
View reviewed changes
Copy link
Contributor Author

@juherr juherr left a comment

Choose a reason for hiding this comment

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

@goekay Please review and give feedbacks :)

pom.xml
<artifactId>ocpp-jaxb</artifactId>
<version>0.0.9</version>
<version>feature~java-time-a74e2a0636-1</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to be replaced before merge

src/test/java/de/rwth/idsg/steve/web/api/TransactionRestControllerTest.java
@@ -191,9 +191,9 @@ public void test8() throws Exception {
.andExpect(jsonPath("$[0].chargeBoxId").value("cb-2"))
.andExpect(jsonPath("$[0].ocppIdTag").value("id-3"))
.andExpect(jsonPath("$[0].ocppIdTag").value("id-3"))
.andExpect(jsonPath("$[0].startTimestamp").value(start.toString()))
.andExpect(jsonPath("$[0].startTimestamp").value("2022-10-01T00:00:00Z"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if it is a issue from the test or the implementation

src/test/java/de/rwth/idsg/steve/web/api/OcppTagsRestControllerTest.java Outdated
@@ -175,7 +175,7 @@ public void test5() throws Exception {
.andExpect(jsonPath("$[0].parentIdTag").value("parent-id-1"))
.andExpect(jsonPath("$[0].inTransaction").value("false"))
.andExpect(jsonPath("$[0].blocked").value("true"))
.andExpect(jsonPath("$[0].expiryDate").value("2020-10-01T00:00:00.000Z"))
.andExpect(jsonPath("$[0].expiryDate").value("2020-10-01T00:00:00"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if it is a issue from the test or the implementation

src/main/java/de/rwth/idsg/steve/utils/DateTimeUtils.java
if (ldt == null) {
return null;
} else {
return ldt.toDateTime();
return ldt.atOffset(ZoneOffset.UTC);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When possible, I think the offset should be taken from the location of the station

Copy link
Contributor Author

@juherr juherr Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Related to #96

@juherr juherr force-pushed the refacto/joda-time branch from 17ae9e7 to bd04b78 Compare April 3, 2025 07:24
@goekay
Copy link
Member

goekay commented Apr 3, 2025 •
edited
Loading

i considered this years ago but then dismissed the topic, because of the following...

joda's DateTime is forgiving if the timezone designator is missing. timezone designator is a MUST for java's data types. you can imagine joda's DateTime as a union of multiple possible date-time types in java.time package. since some weird charging station implementations do NOT send timezone, i decided to stay with joda's DateTime back then. sad but true. otherwise, we would break compatibility.

@goekay
Copy link
Member

goekay commented Apr 3, 2025

test code:

    public static void main(String[] args) {
        // this will work fine for both
        {
            String s = "2025-04-03T09:30:00.00000+02:00";

            DateTime dt1 = DateTime.parse(s);
            System.out.println(dt1);

            OffsetDateTime dt2 = OffsetDateTime.parse(s);
            System.out.println(dt2);
        }

        // this will explode in case of OffsetDateTime
        {
            String s = "2025-04-03T09:30:00";

            DateTime dt1 = DateTime.parse(s);
            System.out.println(dt1);

            OffsetDateTime dt2 = OffsetDateTime.parse(s);
            System.out.println(dt2);
        }
    }

@juherr
Copy link
Contributor Author

juherr commented Apr 3, 2025

You are right, but the issue doesn't exist because already treated. See https://github.com/steve-community/ocpp-jaxb/pull/16/files#r2026501536

@juherr
switch from joda.time to java.time
Loading
Loading status checks…
1a37545 
Close steve-community#98
@juherr juherr force-pushed the refacto/joda-time branch from bd04b78 to 1a37545 Compare April 5, 2025 15:14
@goekay
Copy link
Member

goekay commented Apr 6, 2025

Neat! Let's continue the conversation there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

switch from joda.time to java.time
2 participants
@juherr @goekay