PRP-10968 Introduce TimeZoneUsageCheck
#9
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stephan202
force-pushed
the
anna8712/PRP-10968
branch
from
7ff4063 to
d8a721b
Compare
Stephan202
approved these changes
Feb 14, 2021
| LocalDate.class.getName(), | ||
| LocalDateTime.class.getName(), | ||
| LocalTime.class.getName()) | ||
| .namedAnyOf("now")); |
There was a problem hiding this comment.
Suggested change
| .namedAnyOf("now")); | |
| .named("now")); |
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| name = "TimeZoneUsageCheck", | ||
| summary = "Avoid illegal operations on assorted time related objects", |
There was a problem hiding this comment.
Suggested change
| summary = "Avoid illegal operations on assorted time related objects", | |
| summary = "Derive the current time from an existing `Clock` Spring bean, and don't rely on a `Clock`'s time zone", |
| /** A {@link BugChecker} which flags illegal time-zone related operations. */ | ||
| @AutoService(BugChecker.class) | ||
| @BugPattern( | ||
| name = "TimeZoneUsageCheck", |
There was a problem hiding this comment.
Suggested change
| name = "TimeZoneUsageCheck", | |
| name = "TimeZoneUsage", |
| "clock.instant();", | ||
| "clock.millis();", | ||
| "Clock.offset(clock, Duration.ZERO);", | ||
| "Clock.tick(clock, Duration.ZERO);", |
| } | ||
|
|
||
| @Test | ||
| public void testIdentifyCases() { |
There was a problem hiding this comment.
We generally just combine this into a single test class.
Stephan202
reviewed
Feb 15, 2021
Comment on lines
58
to
61
| if (!IS_BANNED_TIME_METHOD.matches(tree, state)) { | ||
| return Description.NO_MATCH; | ||
| } | ||
| return buildDescription(tree).build(); |
There was a problem hiding this comment.
@rickie why the removed newline? Seems nicer with it :)
Alternatively, let's do:
Suggested change
| if (!IS_BANNED_TIME_METHOD.matches(tree, state)) { | |
| return Description.NO_MATCH; | |
| } | |
| return buildDescription(tree).build(); | |
| return IS_BANNED_TIME_METHOD.matches(tree, state) | |
| ? return buildDescription(tree).build() | |
| : Description.NO_MATCH; |
There was a problem hiding this comment.
The line felt more like a direct else that needed to be close to the if, so I removed the line.
I applied the alternative, way better :) @Stephan202
Stephan202
force-pushed
the
anna8712/PRP-10968
branch
from
b2d642c to
d540c39
Compare
Stephan202
reviewed
Feb 27, 2021
|
^ @anna8712 question about whether to merge this one stands :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new Error Prone plugin which flags bad practices with objects related to
time, which might be critical when we'll handle different time zones in our systems.This check will flag the bad usages, as a build
Warning.