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

fix time formatting error with different timezones #149

Merged
merged 2 commits into from
May 2, 2022
Merged

fix time formatting error with different timezones #149

merged 2 commits into from
May 2, 2022

Conversation

wldhg
Copy link
Contributor

@wldhg wldhg commented Apr 28, 2022

Hello!

The timezone is not parsed with +00:00 format string at discord/time.go#11.
This caused my embed message timestamp to display incorrectly.
So I changed it to support various timezone.
Please check this issue!

Thank you.

@Skye-31 Skye-31 requested a review from topi314 April 28, 2022 07:36
discord/time.go Outdated
@@ -32,7 +32,7 @@ func (t *Time) UnmarshalJSON(data []byte) error {
return err
}

t.Time = parsed
t.Time = parsed.UTC()
Copy link
Member

Choose a reason for hiding this comment

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

does discord always send time in UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems discord does not specify their timezone, because ISO8601 includes timezone information. I just added .UTC() to pass the test. .UTC() just changes timezone in time.Time instance, so the absolute time will not be changed.

Copy link
Member

Choose a reason for hiding this comment

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

perhaps we should instead update the test?

discord/time.go Outdated
@@ -8,7 +8,7 @@ import (
"github.com/disgoorg/disgo/json"
)

const TimeFormat = "2006-01-02T15:04:05+00:00"
const TimeFormat = "2006-01-02T15:04:05-07:00"
Copy link
Member

Choose a reason for hiding this comment

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

after reading a bit more about ISO 8601 it seems like time.RFC3339 should also work?
I'm not rly into this time shit so I appreciate the help

Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

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

I did some closer investigation and it seems like we can just use the std time.Time. So removing the custom time type should be the way to go

the only annoying thing the std lib does not is marshalling a time.IsZero() to null but we can work around that with the json.Nullable[T any] type

@topi314
Copy link
Member

topi314 commented Apr 30, 2022

I have the needed changes laying around locally if you don't mind me pushing them into your pr

@topi314 topi314 added this to the v0.9.0 milestone May 2, 2022
@topi314
Copy link
Member

topi314 commented May 2, 2022

I pushed them now as I'd like to get the changes out. Thank you tho!

@topi314 topi314 merged commit 78fcbf5 into disgoorg:development May 2, 2022
@wldhg
Copy link
Contributor Author

wldhg commented May 5, 2022

I'm so sorry I've been away for a long time because of the move! Thank you for finishing up all the things :)

@topi314
Copy link
Member

topi314 commented May 5, 2022

Sure 👍

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.

3 participants