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

Add Zone property to ZonedClock that returns DateTimeZone of the clock #1362

Closed
rodion-m opened this issue Apr 29, 2019 · 6 comments
Closed

Add Zone property to ZonedClock that returns DateTimeZone of the clock #1362

rodion-m opened this issue Apr 29, 2019 · 6 comments
Assignees

Comments

@rodion-m
Copy link

@rodion-m rodion-m commented Apr 29, 2019

Hi there. It came as a surprise to me that ZonedClock don't have its DateTimeZone public property.
Sometimes we need to get know the timezone of given clock, please consider to add this property:

public DateTimeZone Zone => zone;
@jskeet
Copy link
Member

@jskeet jskeet commented Apr 29, 2019

I'll consider it, but could you give me more information about the use case? (That will help me work out whether we also need to expose the clock and the calendar as well.)

As a workaround for the moment, you could always call GetCurrentZonedDateTime() and check the time zone of that.

Loading

@jskeet jskeet self-assigned this Apr 29, 2019
@rodion-m
Copy link
Author

@rodion-m rodion-m commented Apr 29, 2019

Actually I wanted to use as a part of my extension ToStringRelatively, that returns relatively string of the given interval (for instance Today 14:00 - 15:00), so it requires clock to get current time and timezone to convert Instants to Zoned/Local date time, since I use ZonedClock I had to duplicate time zone, because ZonedClock doesn't have this property.

Loading

@jskeet
Copy link
Member

@jskeet jskeet commented Apr 29, 2019

So does the method actually ask the ZonedClock for the current instant in its time zone? If you're not calling any of the time-zone-specific methods from ZonedClock, I think that taking an IClock and a DateTimeZone separately would be clearer. (It's like if you want a date and a time, but you're not actually referring to a specific LocalDateTime - I wouldn't recommend using LocalDateTime in that scenario just because it happens to combine the two.)

Loading

@jskeet
Copy link
Member

@jskeet jskeet commented May 25, 2019

Any more information on this? I'd really like to understand scenarios where accepting a ZonedClock and then accessing the Zone is really the best approach.

Loading

@rodion-m
Copy link
Author

@rodion-m rodion-m commented May 28, 2019

Hi John! I'm sorry for delay. Actually I solved this issue creating my own class LocalInterval (consisted of two LocalDateTimes) from two Instants like this: LocalInterval.FromInstants(instantStart, instantEnd, tz). So, it solved my use case. If I met similar issue again, I'll post it here. For now it solved. Anyway I think it's worth to open Zone field to ZonedClock, I assume it's logical and it may make life easier to someone.

Loading

jskeet added a commit to jskeet/nodatime that referenced this issue Jun 28, 2019
I hope that most uses of ZonedClock won't need them, but they don't do active harm.

Fixes nodatime#1362
@jskeet jskeet closed this in #1396 Jun 28, 2019
jskeet added a commit that referenced this issue Jun 28, 2019
I hope that most uses of ZonedClock won't need them, but they don't do active harm.

Fixes #1362
cuteant added a commit to cuteant/nodatime that referenced this issue Jun 29, 2019
I hope that most uses of ZonedClock won't need them, but they don't do active harm.

Fixes nodatime#1362
cuteant added a commit to cuteant/nodatime that referenced this issue Jun 29, 2019
I hope that most uses of ZonedClock won't need them, but they don't do active harm.

Fixes nodatime#1362
@rodion-m
Copy link
Author

@rodion-m rodion-m commented Jul 3, 2019

Thank you!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants