-
Notifications
You must be signed in to change notification settings - Fork 563
Sync SyncIO.realTime* methods with Clock implementation
#4522
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
base: series/3.x
Are you sure you want to change the base?
Conversation
|
Hmm... The CI failure doesn't seem relevant to me: |
|
In theory we could split this into 2 parts: the fix, and the new method. The fix could go into 3.6.x. (But I'm not sure it's worth it.) |
|
I can create a separate PR without |
|
Honestly, I'm not sure it's worth the hassle. You seem to be the first to notice the bug, so if you don't need the fix in 3.6.x, then let's leave this as it is. (I mean the PR is fine as it is.) |
| now.getOffset.getTotalSeconds | ||
| ) | ||
|
|
||
| assertCompleteAsSync(op, (true, ZoneOffset.UTC.getTotalSeconds)) |
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.
ZoneOffset.UTC.getTotalSeconds
Is this a fancy way of writing 0?
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.
Yeah, probably you're right.. To be honest, I didn't give it much thought.
Also, I'm not really sure what the point of getting ZonedDateTime instance is when its offset is always 0. But this is how the original method Clock.realTimeZonedDateTime works.
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.
@durban , I changed ZoneOffset.UTC.getTotalSeconds to 0, because it was really unnecessary. But the PR may need yet another approval. Thank you!
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.
Thanks, it was quite fine before; I tend to make remarks that are not crucial, just suggest possible improvements and/or for my own education. 👍
Otherwise,
Clock[SyncIO].realTimeInstantmight not be the same asSyncIO.realTimeInstant.Discussed on Discord.
Also adds
SyncIO.realTimeZonedDateTime, similar toIO.realTimeZonedDateTime.