-
Notifications
You must be signed in to change notification settings - Fork 39
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
Assorted ZoneOffset
improvements
#38
Conversation
(Had a bit of a struggle to come up with a good name for the PR... so ideas welcome 😄) |
Validated downstream, results are promising 😄. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks and straightforward. Nice small improvement. 🚀
82943cf
to
0bfa9e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and added a small commit. Suggested commit message:
Suggest assorted canonical `ZoneOffset` usages (#38)
- Statically import `ZoneOffset.UTC`.
- Prefer `Instant#atOffset` over `OffsetDateTime#ofInstant`.
" ZoneOffset z1 = ZoneOffset.UTC;", | ||
" ZoneOffset z2 = ZoneOffset.MIN;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we can use the identification
test to verify that only one is flagged; the replacement logic is already sufficiently covered.
/** Prefer {@link Instant#atOffset(ZoneOffset)} over the more verbose alternative. */ | ||
static final class InstantAtOffset { | ||
@BeforeTemplate | ||
OffsetDateTime before(Instant instant, ZoneOffset zoneOffset) { | ||
return OffsetDateTime.ofInstant(instant, zoneOffset); | ||
} | ||
|
||
@AfterTemplate | ||
OffsetDateTime after(Instant instant, ZoneOffset zoneOffset) { | ||
return instant.atOffset(zoneOffset); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but something to ponder: ideally we generalize a rule such as this one, such that OffsetDateTime::ofInstant
is also rewritten to Instant::atOffset
. (Or not 🤔.)
Changes LGTM. |
ZoneOffset.UTC
as candidate.Instant#atOffSet
overOffsetDateTime#ofInstant
.