search

alphagov / notifications-java-client

Java client for the GOV.UK Notify API
https://central.sonatype.com/artifact/uk.gov.service.notify/notifications-java-client
MIT License
8 stars 30 forks source link

Migrate from joda time to java.time #202

Closed neilmendum closed 1 year ago

neilmendum commented 3 years ago

What problem does the pull request solve?

Migrate from joda time to java.time to resolve #61

I wasn't sure how you handle breaking API changes so I've only bumped the patch version number.

Checklist

idavidmcdonald commented 3 years ago

Hey @neilmendum, thanks so much for this! We love contributions :) Sorry it's taken a while to get back to you though :(

So I'm not a java expert but from what I've gathered, the change from joda time to java.time is a sensible one that we support.

The bit I'm less sure on, through my own lack of knowledge of Java, is the change to the public interface from DateTime to PublicDateTime and the other equivalent ones like this. This would be something that we'd consider a breaking change and a major version bump.

Do you see this change of naming as something that is essential or would be misleading if we didn't do it? Any extra context on that that you are able to give would be much appreciated.

Note, we are pretty swamped with things so I can't guarantee this is going to get a proper review too soon as we have some other work we are focussing on at the moment. Sorry for that but I wanted to say thanks again because we really do appreciate this!

neilmendum commented 3 years ago

Hi @idavidmcdonald thanks for taking a look! Apologies for taking a while to reply.

I was thinking on your comment:

The bit I'm less sure on, through my own lack of knowledge of Java, is the change to the public interface from DateTime to PublicDateTime and the other equivalent ones like this. This would be something that we'd consider a breaking change and a major version bump.

I have updated LocalDateTime to ZonedDateTime because I realised that is the java.time equivalent of Joda's DateTime. You can read more about it here.

Appreciate that you folks are swamped and may not get to this soon.

neilmendum commented 1 year ago

Closing as it has been open for over a year and the issue hasn't had activity so not priority to fix