-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
cli: Display datetimes in local timezone #561
Conversation
0e753c2
to
2abd60d
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.
The changes look good to me. I am wondering though: It never mentions the time zone. How is this tool expected to work: should you always see the times in the local time zone? Should you see it in the event's time zone? What if there are many time zones - then it does not make sense to see e.g. 19:00 as it could mean a lot.
Previously the start and end datetimes were always printed out in the timezone that they appear in the calendar entry. In 7a8d584 duration support was added and an attempt was already made to display the datetime in the local timezone. Unfortunately in that specific case the `start.astimezone(start.tzinfo)` is a no-op and does absolutely nothing. Unlike the name suggests, `astimezone()` adjusts the date and time data, such that they match the passed tzinfo, but since that is the same timezone data as before, nothing changes. [0] In order to properly convert to the user's local timezone, we need to call the method with no arguments. With this the timezone is always properly displayed, which makes up for a much nicer UX for users of the cli. The test has to be adapted to expect the datetime in the local timezone, hence we cannot hardcode the entire expected string anymore. [0] https://docs.python.org/3/library/datetime.html#datetime.datetime.astimezone
Yes
No, that wouldn't make any sense. If someone shares their calendar in Google Calendar with me, I expect Google Calendar to show me all events with the time of my own timezone, not the timezone of the other person.
I don't know what you mean with many time zones, the time is always displayed in one timezone, the local timezone of the user. Theoretically we could append the timezone description to the time, but that would be pretty unusual. Usually you only add the timezone description, if it is not already in the local timezone of the user. By the way before this patch the CLI tool also never showed the timezone info, it just displayed no timezone despite the timezone of the displayed time being not the local timezone. That was pretty surprising behaviour. Just as an example, to make sure we are on the same page: Now before this patch, the CLI tool would show this for the user living in the
Note how there is no timezone info, so the user would mistakenly assume that the event starts at 9:30 in their own timezone (since they don't know what the timezone of the other person is). But in reality the event starts 14:30 in their own timezone. Now after this patch, the CLI tool outputs the expected:
Which is the correct time in the user's timezone. |
2abd60d
to
1e0c78f
Compare
This makes sense to me, too. Thanks for providing this! |
And in case you were wondering where the hell people are using the icalendar CLI tool in "production", many people use it in CLI mail clients (e.g. mutt) to display sent calendar events inline directly in the mail without having to leave the terminal. That's also why the local timezone makes sense. |
This is live in v5.0.11. Thanks for your contributions! |
This makes up for a much nicer UX for users of the cli utility.
Before this patch the start and end datetimes were always displayed with a fixed timezone (the timezone of the calendar creator).
This meant that users had to manually calculate the time in their own timezone.
With this patch the times are printed in the expected format, i.e. in the user's own local timezone.
It seems that in 7a8d584 an attempt was already made to implement this, but unfortunately
start.astimezone(start.tzinfo)
is a no-op and does absolutely nothing. Instead no arguments have to be passed to theastimezone()
function to convert to the user's timezone, see https://docs.python.org/3/library/datetime.html#datetime.datetime.astimezoneThe unit test has been adapted to check for the output in the correct timezone.