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

Remove DateTypeAdapter to avoid code duplication #2546

Merged

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Nov 19, 2023

Purpose

Remove the internal DateTypeAdapter class to avoid code duplication

Description

The same functionality exists in DefaultDateTypeAdapter (with some slight overhead though), and there is currently code duplication between these classes.

The code in DefaultDateTypeAdapter and the way it is used now as replacement should behave the same way DateTypeAdapter did, but feel free to double-check in case I overlooked something.

Checklist

  • New code follows the Google Java Style Guide
    This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
    Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

The same functionality exists in `DefaultDateTypeAdapter` (with some slight
overhead), and there is currently code duplication between these classes.
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

This is great, thanks! I think I've said before that the way it handles Date is probably Gson's biggest mistake. We can't easily fix that, but we can at least make it easier to improve the situation.

The changes look good to me. I'm running this past Google's tests in case that turns anything up.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Google's internal tests all passed.

@eamonnmcmanus eamonnmcmanus merged commit ea4ffde into google:main Nov 19, 2023
10 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/remove-DateTypeAdapter branch November 22, 2023 21:42
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Sep 14, 2024
The same functionality exists in `DefaultDateTypeAdapter` (with some slight
overhead), and there is currently code duplication between these classes.
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