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

Introduce additional matchers to TimeZoneUsage rule #311

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

chamil-prabodha
Copy link
Contributor

The OffsetDateTime, OffsetTime and ZonedDateTime classes implement methods such as now(), now(Clock clock), now(ZoneId zone) which either rely on the system clock or retrieve the time zone in the clock variable. The calls to the aforementioned methods should be avoided as per the definition of TimeZoneUsage rule.

This PR detects the usage of such violations and produces warnings during the compilation.

@chamil-prabodha chamil-prabodha changed the title Introduce additional matchers TimeZoneUsage rule Introduce additional matchers to TimeZoneUsage rule Oct 24, 2022
@chamil-prabodha
Copy link
Contributor Author

chamil-prabodha commented Oct 24, 2022

Suggested commit message:

Introduce additional matchers to `TimeZoneUsage` check (#311)

Add matchers for `OffsetDateTime`, `OffsetTime` and `ZonedDateTime` classes.

@nathankooij
Copy link
Contributor

I think it looks good in principle, but I worry about the migration effort that this will put on the WMS team, which will hurt adoption. What is the expected migration path for them, and can we help ease their pain? (They are big users of OffsetDateTime.now(...).)

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Added a commit with some unrelated cleanup. Nice improvements @chamil-prabodha !

Created an alternative suggested commit message:

Have `TimeZoneUsage` check flag `{OffsetDate,Offset,ZonedDate}Time#now` (#311)

@Stephan202 Stephan202 added this to the 0.5.0 milestone Oct 24, 2022
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

W.r.t. OffsetDateTime.now(clock) usages: Internally we use UTC clocks, so I think that would become clock.instant().atOffset(UTC), which isn't that bad.

chamil-prabodha and others added 2 commits October 25, 2022 16:49
Add matchers for `OffsetDateTime`, `OffsetTime` and `ZonedDateTime` classes.
@Stephan202 Stephan202 force-pushed the chamil-prabodha/PSM-1680 branch from ea4fce0 to 827b70c Compare October 25, 2022 14:49
@Stephan202 Stephan202 merged commit 2138827 into master Oct 25, 2022
@Stephan202 Stephan202 deleted the chamil-prabodha/PSM-1680 branch October 25, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants