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

Change uint to float64 for TimeStamps #470

Closed
wants to merge 1 commit into from

Conversation

MaxBosse
Copy link
Contributor

@MaxBosse MaxBosse commented Oct 30, 2017

Hey,

currently the develop branch will throw the following error when parsing a PRESENCE_UPDATE containing a Game object:
2017/10/30 14:52:48 [DG0] wsapi.go:477:onEvent() error unmarshalling PRESENCE_UPDATE event, json: cannot unmarshal number 1509371565406.0 into Go struct field TimeStamps.start of type uint

This change will fix the error by changing the TimeStamps.start and TimeStamps.end values from an uint to a float64.

@njhanley
Copy link
Contributor

njhanley commented Nov 7, 2017

Why use a float64 when the payload is an int64?
https://discordapp.com/developers/docs/topics/rich-presence#updating-presence-update-presence-payload-fields

@iopred
Copy link
Collaborator

iopred commented Nov 7, 2017

Breaking the API is not the right way to fix this, writing a custom UnmarshalJSON seems right here.

@Hinara Hinara mentioned this pull request Nov 8, 2017
@njhanley
Copy link
Contributor

njhanley commented Nov 9, 2017

My previous comment was incorrect in assuming the Discord Rich Presence SDK payload was reflective of the Discord API. I see now that Discord returns the "timestamps" object's fields as integers encoded as floats.

I agree that a custom UnmarshalJSON makes sense but the API still must be changed as a uint is 32-bit on some platforms. I've created a pull request (#475) with the changes I believe are necessary.

@MaxBosse MaxBosse closed this Nov 28, 2017
@bwmarrin bwmarrin added this to the N/A milestone Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants