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

Improved Date Decoding #9

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Conversation

charxene
Copy link
Contributor

@charxene charxene commented Jun 1, 2022

decode timestamps as Date objects

@cryptoAlgorithm
Copy link
Member

cryptoAlgorithm commented Jun 1, 2022

Ok hold on, I'd rather you modify the 1 commit with a more descriptive message, than create a fresh PR with one commit. I thought this PR was to resolve the issues with #6, turns out its completely separate. My bad!

Comment on lines +10 to +20
let iso8601 = { () -> ISO8601DateFormatter in
let fmt = ISO8601DateFormatter()
fmt.formatOptions = [.withInternetDateTime]
return fmt
}()

let iso8601WithFractionalSeconds = { () -> ISO8601DateFormatter in
let fmt = ISO8601DateFormatter()
fmt.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
return fmt
}()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

annoyingly if you specify with fractional seconds, you must use fractional seconds, and the discord api is inconsistent with this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah thats the annoying part. Another problematic thing is the nonce value in Message. It can either be an int or a string according to the dev docs, so I have no idea how to add that property.

Copy link
Member

Choose a reason for hiding this comment

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

Overall this PR seems quite well done, would be great if you could resolve the issues with #6

@cryptoAlgorithm cryptoAlgorithm changed the title date decoding Improved Date Decoding Jun 1, 2022
@cryptoAlgorithm
Copy link
Member

I see that you're using a custom decoding strategy to decode the Dates. Could you also see if you can set the keyDecodingStrategy to .convertFromSnakeCase while you're at it? This way, the properties can be better aligned with the naming style of the code.

@tonyarnold
Copy link
Contributor

It might be better to do the snake case to camel case conversion in a separate PR? That's going to be a lot of churn.

@cryptoAlgorithm
Copy link
Member

Good point. I'll wait for this PR to be merged, then work on that. I don't want to deal with tons of merge conflicts xD

@charxene charxene marked this pull request as ready for review June 2, 2022 01:53
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.

3 participants