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

Replace ZoneId.of("UTC") with ZoneOffset.UTC #3988

Closed
wants to merge 1 commit into from

Conversation

pkoenig10
Copy link
Contributor

This PR updates the ZoneIdOfZ check to also replace ZoneId.of("UTC").

"TestClass.java",
"import java.time.ZoneId;",
"public class TestClass {",
" private static final ZoneId UTC = ZoneId.of(\"utc\");",
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge deal, but http://errorprone.info/bugpattern/InvalidZoneId should prevent this from compiling in the first place :-)

@@ -57,7 +57,7 @@ public final class ZoneIdOfZ extends BugChecker implements MethodInvocationTreeM
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (ZONE_ID_OF.matches(tree, state)) {
String zone = constValue(tree.getArguments().get(0), String.class);
if (zone != null && zone.equals("Z")) {
if (zone != null && (zone.equals("Z") || zone.equals("UTC"))) {
Copy link
Member

Choose a reason for hiding this comment

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

So technically, ZoneId.of("UTC") is not equal to ZoneOffset.UTC. They have different values for getId(), (the former is "Z" and the latter is "UTC" I believe)...whether this matters in practice, I'm not sure.

Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I did not realize that, thanks for pointing this out. Let me know if you'd prefer not to merge this because of this potentially breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Yea let's revert this to be on the safe side, especially since this is enabled as an error, not just a warning.

@pkoenig10 pkoenig10 closed this Jun 27, 2023
@pkoenig10 pkoenig10 deleted the zoneIdUtc branch June 27, 2023 14:53
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