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 DateFormat time zone is not restored. #2548

Closed
wants to merge 4 commits into from
Closed

Fix DateFormat time zone is not restored. #2548

wants to merge 4 commits into from

Conversation

Carpe-Wang
Copy link
Contributor

@Carpe-Wang Carpe-Wang commented Nov 19, 2023

Purpose

Fix DateFormat time zone is not restored after parsing. About issue is #2547

Description

Before attempting to parse the date string, save the current timezone for each DateFormat object. Use a finally block to ensure that the original timezone is restored for each DateFormat object.

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.

This looks good, but can you add a test that fails without this change and passes with it?

import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

Your IDE probably did this for you, but please undo it. :-)
This project follows the Google Java Style Guide, notably the bit excluding wildcard imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, Idea change it automatically.

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Nov 19, 2023

Just as forewarning: This change conflicts with #2546, so one of the PRs will cause conflicts for the other one.

Also this affects more classes:

  • DefaultDateTypeAdapter

  • SQL type adapters:

    • SqlDateTypeAdapter
    • SqlTimeTypeAdapter

    (they might not actually be affected because their pattern does not include the time zone, but might be good nonetheless to restore the DateFormat time zone, just to be safe)

If you don't want to change these yet, I can do that in a follow-up PR.

Some additional notes:

  • Please adjust the title of this PR; "fix issue##2547" is not really expressive when someone at a glance tries to understand what the PR or eventually the commit on main did; what about this?

    Fix `DateFormat` time zone is not restored after parsing

  • Please edit the description and write "Fix #2547" somewhere in it; GitHub understands certain keywords and then automatically links issue and PR; otherwise it does not understand that this PR fixes the GitHub issue

@Carpe-Wang Carpe-Wang changed the title fix issue##2547 Fix DateFormat time zone is not restored. Nov 19, 2023
@Carpe-Wang
Copy link
Contributor Author

Just as forewarning: This change conflicts with #2546, so one of the PRs will cause conflicts for the other one.

Also this affects more classes:

  • DefaultDateTypeAdapter

  • SQL type adapters:

    • SqlDateTypeAdapter
    • SqlTimeTypeAdapter

    (they might not actually be affected because their pattern does not include the time zone, but might be good nonetheless to restore the DateFormat time zone, just to be safe)

If you don't want to change these yet, I can do that in a follow-up PR.

Some additional notes:

ok, I change title and pr's description. About other classes need to be changed, I will do it in next week.

@Carpe-Wang
Copy link
Contributor Author

This looks good, but can you add a test that fails without this change and passes with it?

yes, I add the test, and find my change can not fix the bug. I will debug to find how to fix.

@Carpe-Wang
Copy link
Contributor Author

there are some issue in my local Idea, I can’t fetch new branch. I will close the issue and make a new pr when I finished the development.

@Carpe-Wang Carpe-Wang closed this Nov 20, 2023
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.

3 participants