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

Fix GsonBuilder.setDateFormat ignoring partial DEFAULT; deprecate setDateFormat(int) #2556

Conversation

Marcono1234
Copy link
Collaborator

Purpose

Resolves #1529

Description

The main issue was this line:

} else if (dateStyle != DateFormat.DEFAULT && timeStyle != DateFormat.DEFAULT) {

When one style was non-DEFAULT but the other was DEFAULT (e.g. dateStyle=SHORT, timeStyle=DEFAULT), then GsonBuilder would ignore the styles and use DEFAULT, DEFAULT. That is incorrect because it leads to a different date pattern.
This affected both setDateFormat(int) and setDateFormat(int, int) of GsonBuilder.

However, setDateFormat(int) is a bit counterintuitive: Since it only takes a 'date style' you would expect it to use DateFormat.getDateInstance. However, that is not actually the case, it uses DateFormat.getDateTimeInstance with the 'time style' being the value which has been set previously with GsonBuilder.setDateFormat(int, int). See also #1529 (comment) for more details.

Arguably this could have been fixed by changing setDateFormat(int) to use DateFormat.getDateInstance, as one would expect. But since we don't encourage users to use the locale specific formatted dates anymore (see e.g. #2546 (review)) I thought it would be easier to just deprecate the method, especially since it was not working properly anyway due to #1529 (which is fixed by this PR now though).

This pull request also removes internal code which handled the 'date style'-only case, but which was never actually called from the main code but only from tests.

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

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.

I ran this against all of Google's internal tests and there were no failures.

@eamonnmcmanus eamonnmcmanus merged commit 13be1d1 into google:main Mar 1, 2024
11 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/GsonBuilder-setDateFormat-DEFAULT branch March 1, 2024 22:41
tibor-universe pushed a commit to getuniverse/gson that referenced this pull request Sep 14, 2024
…setDateFormat(int)` (google#2556)

* Fix `GsonBuilder.setDateFormat` ignoring partial DEFAULT; deprecate `setDateFormat(int)`

* Remove date format methods not used by main code

* Adjust example
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.

GsonBuilder.setDateFormat(int) is ignored
2 participants