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.

Sign up for GitHub

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

Jump to bottom

Encode timestamps in RFC3339 #2665

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Encode timestamps in RFC3339 #2665

merged 1 commit into from
Dec 22, 2023

Conversation

Kerollmops
Copy link

This PR fixes #2664, but I am unsure if it should be unwrapped. It would be better to output an error if we cannot encode them. Also, please tell me if it is ok to encode in RFC3339 instead of ISO 8601.

@github-actions github-actions bot added model Related to the `model` module. http Related to the `http` module. labels Dec 17, 2023
mkrasnitski
mkrasnitski requested changes Dec 20, 2023
View reviewed changes
Copy link
Collaborator

@mkrasnitski mkrasnitski left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! A few review concerns:

  1. This PR changes the signatures of public-facing functions and is therefore breaking. As such, it should target the next branch rather than current.
  2. According to Discord's docs, the List Joined Private Archived Threads endpoint expects the before parameter to be a snowflake rather than a timestamp. Since we're dealing with threads here, I think ChannelId would be the right type to use.
  3. You've missed an endpoint here, in particular the List Private Archived Threads endpoint, which actually does expect a formatted timestamp.

src/http/client.rs Outdated Show resolved Hide resolved
src/http/client.rs Outdated Show resolved Hide resolved
src/model/channel/channel_id.rs Outdated Show resolved Hide resolved
@Kerollmops Kerollmops force-pushed the encode-before-rfc3339 branch 2 times, most recently from 34b5d8e to 932871c Compare December 21, 2023 11:01
@Kerollmops Kerollmops changed the base branch from current to next December 21, 2023 11:01
@Kerollmops
Copy link
Author

Thank you for your review 🙏

I rebased on the next branch and used ChannelIds instead of Timestamps or u64s.

@mkrasnitski
Copy link
Collaborator

Ah, sorry for not being clear enough. It's only the List Joined Private Archived Threads endpoint that takes a snowflake. The other two endpoints do indeed take timestamps.

@Kerollmops
Copy link
Author

Oups, sorry! I just fixed this an squashed the commits.

@arqunis arqunis added fix A solution to an existing bug. breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users labels Dec 22, 2023
@arqunis arqunis merged commit 7ccc2f2 into serenity-rs:next Dec 22, 2023
22 checks passed
@Kerollmops Kerollmops deleted the encode-before-rfc3339 branch December 22, 2023 21:04
@mkrasnitski mkrasnitski mentioned this pull request Dec 23, 2023
Closed
arqunis pushed a commit to arqunis/serenity that referenced this pull request Dec 28, 2023
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
756ae5f 
Fixes serenity-rs#2664
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 2, 2024
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
3bcb1cb 
Fixes serenity-rs#2664
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 5, 2024
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
952e75f 
Fixes serenity-rs#2664
@jamesbt365 jamesbt365 mentioned this pull request Jan 9, 2024
Closed
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 16, 2024
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
b233932 
Fixes serenity-rs#2664
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 22, 2024
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
226d853 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Feb 9, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
c38603e 
Fixes #2664
arqunis pushed a commit to arqunis/serenity that referenced this pull request Mar 1, 2024
@Kerollmops @arqunis
Encode timestamps in RFC3339 (serenity-rs#2665)
b23115e 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Mar 10, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
72417c3 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request Mar 11, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
d6dc322 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request Mar 11, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
7d827ad 
Fixes #2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Mar 13, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
11a948f 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Mar 13, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
8875880 
Fixes #2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Mar 19, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
c1209bf 
Fixes serenity-rs#2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Mar 19, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
29634dd 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Mar 21, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
010e224 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request Mar 25, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
1bb54b9 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request Mar 29, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
bb44458 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request Mar 31, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
abdb2d9 
Fixes #2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Mar 31, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
0a33c6f 
Fixes serenity-rs#2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Apr 1, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
962567c 
Fixes serenity-rs#2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request May 14, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
a8d39c7 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request May 14, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
8238f64 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request May 23, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
191bac3 
Fixes #2664
GnomedDev pushed a commit that referenced this pull request May 28, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
02a9de7 
Fixes #2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Jun 9, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
f20e8fc 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Jun 22, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
308a210 
Fixes #2664
GnomedDev pushed a commit to GnomedDev/serenity that referenced this pull request Jun 22, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (serenity-rs#2665)
78968aa 
Fixes serenity-rs#2664
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 29, 2024
@Kerollmops @mkrasnitski
Encode timestamps in RFC3339 (serenity-rs#2665)
cd4f833 
Fixes serenity-rs#2664
mkrasnitski pushed a commit to mkrasnitski/serenity that referenced this pull request Jul 30, 2024
@Kerollmops @mkrasnitski
Encode timestamps in RFC3339 (serenity-rs#2665)
cb3ecaf 
Fixes serenity-rs#2664
GnomedDev pushed a commit that referenced this pull request Aug 16, 2024
@Kerollmops @GnomedDev
Encode timestamps in RFC3339 (#2665)
71070f5 
Fixes #2664
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arqunis arqunis arqunis approved these changes

@mkrasnitski mkrasnitski mkrasnitski approved these changes

Assignees
No one assigned
Labels
breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users fix A solution to an existing bug. http Related to the `http` module. model Related to the `model` module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The get_channel_archived_public_threads method doesn't encode the timestamp well
3 participants
@Kerollmops @mkrasnitski @arqunis