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

feat: backport guild forum support to v13 #8651

Merged
merged 31 commits into from Jan 2, 2023
Merged

feat: backport guild forum support to v13 #8651

merged 31 commits into from Jan 2, 2023

Conversation

aiko-chan-ai
Copy link

@aiko-chan-ai aiko-chan-ai commented Sep 21, 2022 •
edited

Please describe the changes this PR makes and why it should be merged:

image

#7791

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@vercel
Copy link

vercel bot commented Sep 21, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
discord-js ❌ Failed (Inspect) Dec 27, 2022 at 5:46AM (UTC)

@vercel vercel bot temporarily deployed to Preview September 21, 2022 12:45 Inactive
@vercel vercel bot temporarily deployed to Preview September 21, 2022 13:08 Inactive
@Jiralite
Copy link
Member

Jiralite commented Sep 21, 2022 •
edited

This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

What's the breaking change? If there is one, this cannot be merged.

jaw0r3k
jaw0r3k reviewed Sep 21, 2022
View reviewed changes
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@jaw0r3k
Copy link
Contributor

jaw0r3k commented Sep 21, 2022 •
edited

also can u add
#8645, #8643, #8638, #8646

@aiko-chan-ai
Copy link
Author

also can u add #8645, #8643, #8638

sure :3

@aiko-chan-ai
Copy link
Author

This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

What's the breaking change? If there is one, this cannot be merged.

probably not :c

@vercel vercel bot temporarily deployed to Preview September 21, 2022 14:58 Inactive
@Jiralite Jiralite added this to the discord.js v13.11 milestone Sep 21, 2022
jaw0r3k
jaw0r3k suggested changes Sep 21, 2022
View reviewed changes
src/managers/GuildForumThreadManager.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview September 21, 2022 15:53 Inactive
@JJRcop
Copy link
Contributor

JJRcop commented Sep 21, 2022

probably not :c

Can you be more clear on this? What is the breaking change?

A "breaking change" means the modifications will cause old code written for the previous version to not function the same or create errors. Does this pull request have any of that? Please tell us!

@aiko-chan-ai
Copy link
Author

aiko-chan-ai commented Sep 21, 2022 •
edited

Can you be more clear on this? What is the breaking change?

A "breaking change" means the modifications will cause old code written for the previous version to not function the same or create errors. Does this pull request have any of that? Please tell us!

i changed the name of the Class ThreadManager, same as v14. Still working the same way

@jaw0r3k
Copy link
Contributor

jaw0r3k commented Sep 21, 2022

Can you be more clear on this? What is the breaking change?
A "breaking change" means the modifications will cause old code written for the previous version to not function the same or create errors. Does this pull request have any of that? Please tell us!

i changed the name of the Class ThreadManager, same as v14. Still working the same way

name is the same
just there is more managers

@aiko-chan-ai
Copy link
Author

name is the same just there is more managers

yep

Jiralite
Jiralite reviewed Dec 16, 2022
View reviewed changes
src/managers/GuildForumThreadManager.js Outdated Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
@aiko-chan-ai aiko-chan-ai mentioned this pull request Dec 18, 2022
Closed
3 tasks
@nk980113
Copy link

Nice
It can get merged
Looking forward to version 13.13

kyranet
kyranet approved these changes Dec 25, 2022
View reviewed changes
Copy link
Member

@kyranet kyranet left a comment •
edited

Choose a reason for hiding this comment

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

As a general review, we should not use resolveAutoArchiveMaxLimit in new places. We should also tweak the function (in this PR or another one) to always return the maximum value, as they're not longer boost-locked.

I can maybe accept 'MAX' as a valid option because v13, but it should be discouraged and deprecated (possibly with a deprecation warning). This can be achieved by modifying the function itself, so you don't have to modify multiple files.

However, because that's outside of this PR's scope, I'm approving this anyways. Feel free to make a follow-up PR addressing this review, though.

@aiko-chan-ai
Copy link
Author

hmm, do you mean the SEVEN_DAY_THREAD_ARCHIVE feature can be used on all servers?

@kyranet
Copy link
Member

kyranet commented Dec 26, 2022 •
edited

That's right — discord/discord-api-docs#4825 — can't find a blog post or an ddevs announcement for this, but I just tried setting in a test guild of mine (without boosts) to 7 days and it worked just fine. The test guild also doesn't have THREE_DAY_THREAD_ARCHIVE or SEVEN_DAY_THREAD_ARCHIVE.

kyranet
kyranet reviewed Dec 26, 2022
View reviewed changes
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Just one small docs thing 👍🏼

src/structures/Message.js Outdated Show resolved Hide resolved
@aiko-chan-ai
Copy link
Author

Just one small docs thing 👍🏼

I'm not good at writing docs 😢

@kyranet
Copy link
Member

kyranet commented Dec 26, 2022

Last commit looks good, but as I said in the discord.js guild, you need to change the doc because 'MAX' always yields 7 days and does not longer read guild features anymore.

@nk980113
Copy link

so when will it get merged?

@iCrawl iCrawl merged commit 546ac43 into discordjs:v13 Jan 2, 2023
@aiko-chan-ai aiko-chan-ai deleted the v13 branch January 2, 2023 15:34
@aiko-chan-ai aiko-chan-ai restored the v13 branch January 2, 2023 15:35
@PlavorSeol PlavorSeol mentioned this pull request Mar 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyranet kyranet kyranet approved these changes

@NotSugden NotSugden NotSugden left review comments

@Jiralite Jiralite Jiralite requested changes

@thehairy thehairy thehairy left review comments

@jaw0r3k jaw0r3k jaw0r3k approved these changes

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl iCrawl approved these changes

@SpaceEEC SpaceEEC Awaiting requested review from SpaceEEC

Assignees
No one assigned
Labels
api changes backport packages:discord.js semver:minor
Projects
discord.js
Archived in project
Milestone
discord.js 13.13.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@aiko-chan-ai @Jiralite @jaw0r3k @JJRcop @thehairy @nk980113 @kyranet @vladfrangu @iCrawl @NotSugden