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

Update some internal classes to use Java 8 java.time #2762

Closed

Conversation

panic08
Copy link
Contributor

@panic08 panic08 commented Oct 15, 2024

As of late, we have gotten support for java 8. In this PR, I added java.time package support for ISO8601Utils, SqlDateTypeAdapter, SqlTimeTypeAdapter, UtcDateTypeAdapter classes.

I also added a TODO to DefaultDateTypeAdapter. Since I encountered some problems, such as that with SimpleDateFormat the date becomes longer and passes under tests, and additional logic needs to be written to support java.time, I decided to install this TODO. In the future it will be possible to figure out and add support for java.time and this class.

@panic08 panic08 force-pushed the update-internal-classes-to-use-java-time branch from f6fc336 to dcaa72c Compare October 15, 2024 20:44
@panic08
Copy link
Contributor Author

panic08 commented Oct 15, 2024

It might be better to put the java.time support for each class in a separate commit

@panic08 panic08 force-pushed the update-internal-classes-to-use-java-time branch from dcaa72c to d26d183 Compare October 15, 2024 20:50
@Marcono1234
Copy link
Collaborator

Thanks! I think for UtcDateTypeAdapter and ISO8601Utils it might be best for now to keep them as they are. It is quite likely that their behavior differs slightly from java.time, so this risks introducing compatibility issues. And (unlike Java's SimpleDateFormat) they are already thread-safe, so from a performance perspective these classes are probably fine.

The main problem is Android compatibility (see also "Check Android compatibility" workflow): While we have Java 8 now as minimum version, Gson still targets Android API level 21 as minimum (see README). However, the java.time package was only added in API level 26, see https://developer.android.com/reference/java/time/package-summary. The Android Gradle Plugin supports desugaring of these classes, but not all users might have enabled this.

So I think it is unlikely that we will use the java.time classes in any of the existing adapters in the near future.

There are vague plans to add built-in adapters for java.time classes, but that would probably require reflection to check if these classes are available (see also #2744 (review)), and requires experimentation to see if that approach actually works properly for Android builds (and probably also for a GraalVM Native Image build).

(This is only my personal view on this, I am not a direct member of this project.)

@panic08
Copy link
Contributor Author

panic08 commented Oct 16, 2024

I agree, I think then it will be worth thinking about this update in the future when (if) gson will support android 27

@panic08 panic08 closed this Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants