-
Notifications
You must be signed in to change notification settings - Fork 793
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
Threads #922
Threads #922
Conversation
I think that's everything. |
Updated the code to be in line with these changes on the threads docs: discord/discord-api-docs@663dc59 |
Breaking change!Webhooks now have an optional Because of the way Lemme know if there should be another/better approach to this. |
It looks like this is being pushed out publicly now - what would it take to get this merged? |
There were a few changes to the discord API since I last updated this PR (nothing major, though). This is why this is still a draft. I've been busy lately, that's why I haven't finished this. But I will finish and undraft this PR later this week. |
I think this is ready for review now. @CarsonHoffman |
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just lurking around since I'm waiting for this feature in the upstream and decided to give the PR a look.
Should not the method take into account AddedMembers
field too? The documentation of the method says it updates the member object of the thread
but it only takes care of the case when the user was removed
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right! Will push an update. Let me know what you think.
restapi.go
Outdated
// ThreadEdit edits the given thread | ||
// threadID : The ID of a Thread | ||
// name : The new name to assign the thread. | ||
func (s *Session) ThreadEdit(channelID string, name string) (*Channel, error) { | ||
return s.ThreadEditComplex(channelID, &ThreadEditData{ | ||
Name: name, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that ThreadEditComplex
clears out all properties not specified, I'm not sure that having this function as is here makes a ton of sense. It seems to me like it would quite quickly lead to unexpected behavior and not really have much use.
structs.go
Outdated
type ThreadMember struct { | ||
ID string `json:"id,omitempty"` | ||
UserID string `json:"user_id,omitempty"` | ||
JoinTimeStamp Timestamp `json:"join_timestamp"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably stick to JoinTimestamp
to match ArchiveTimestamp
.
structs.go
Outdated
// Defaults to `PRIVATE_THREAD` in order to match the behavior | ||
// when thread documentation was first published. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this comment is really relevant here given that we don't specify omitempty
; there's no way for the "default" behavior on the API side to take effect. Perhaps having omitempty
might make sense?
restapi.go
Outdated
// ActiveThreads returns all active threads in the channel | ||
// GET /channels/{channel.id}/threads/active | ||
func (s *Session) ActiveThreads(channelID string) (st *ThreadsResponseBody, err error) { | ||
body, err := s.RequestWithBucketID("GET", EndpointChannelActiveThreads(channelID), nil, EndpointThreadMembers("")) | ||
if err != nil { | ||
return | ||
} | ||
|
||
err = unmarshal(body, &st) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this endpoint is already deprecated. We might just want to move to the guild active threads endpoint to begin with; that one also has a more reasonable response structure, such that we could return a slice of threads and a slice of members rather than a custom struct specifically for one function.
Edit: it looks like we already have that one in this PR! I'd just remove this and ThreadsResponseBody
, to be honest.
// Voice region ID for the voice channel, automatic when set to null | ||
RTCRegion *string `json:"rtc_region"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this field is replacing Region
, so we should probably remove that one while we're at it.
restapi.go
Outdated
// ListActiveThreads returns all active threads in the guild, | ||
// including public and private threads. | ||
// Threads are ordered by their `id`, in descending order. | ||
func (s *Session) ListActiveThreads(guildID string) (st *ListActiveThreadsResponseBody, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just return the threads and members as two return values, rather than having ListActiveThreadsResponseBody
exported just for this function.
@CarsonHoffman I think that is everything you requested. |
structs.go
Outdated
|
||
// Constants for the Video Quality Modes of a channel | ||
const ( | ||
VideoQualityModeAuto VideoQualityMode = iota + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is small, but please avoid using iota
for constants defined by Discord. We've run into many instances in the past where changes on Discord's end have gone awry doing this, especially when they add gaps between values (which is not uncommon). You'll notice that none of the other blocks of constants that you changed in this PR use iota
, either.
restapi.go
Outdated
if threadID != "" { | ||
v.Set("thread_id", threadID) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to add the thread ID in, as it's not part of the body; it's a query parameter.
webhook.go
Outdated
@@ -35,6 +36,7 @@ type WebhookParams struct { | |||
Components []MessageComponent `json:"components"` | |||
Embeds []*MessageEmbed `json:"embeds,omitempty"` | |||
AllowedMentions *MessageAllowedMentions `json:"allowed_mentions,omitempty"` | |||
ThreadID string `json:"thread_id,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the thread ID isn't part of the body, we should just have `json:"-"`
here.
state.go
Outdated
return err | ||
} | ||
|
||
if len(t.ChannelIDs) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs note that channel_ids
could be omitted, in which case all channels for the guild are being synced. Should we really be doing nothing and returning ErrStateNotFound
if this is the case?
state.go
Outdated
defer s.Unlock() | ||
|
||
for _, parentChannelID := range t.ChannelIDs { | ||
for _, stateChannel := range s.channelMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems really nasty—it's O(nm), where n is the list of all channels the bot has access to and m is the number of channels being synced. e.g. if the bot can see 2000 guilds, each has 20 channels, and 10 channels were synced (these numbers seem somewhat reasonable for a larger bot), we just looped four hundred thousand times. Perhaps we could limit the search to only the indicated guild's channels? We're already looping through g.Channels
below, and we can delete from channelMap
without looping through it (since the keys match).
Updated it @CarsonHoffman |
state.go
Outdated
OUTER: | ||
for _, c := range g.Channels { | ||
if !c.IsThread() || c.ThreadMetadata.Archived { | ||
g.Channels[index] = c | ||
index++ | ||
continue OUTER | ||
} | ||
|
||
if len(t.ChannelIDs) == 0 { | ||
delete(s.channelMap, c.ID) | ||
continue OUTER | ||
} | ||
|
||
for _, parentChannelID := range t.ChannelIDs { | ||
if c.ParentID == parentChannelID { | ||
delete(s.channelMap, c.ID) | ||
continue OUTER | ||
} | ||
} | ||
|
||
g.Channels[index] = c | ||
index++ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A labeled loop with three different (and similar) continue
points seems really difficult to reason about to me. I would suggest inverting the loop—first loop over the thread sync event's channels, and then do the guild channels in the inner loop, since you'll then be starting the guild channels loop anew each time and won't need to worry about mutating in place.
Updated @CarsonHoffman |
state.go
Outdated
} | ||
} | ||
|
||
return ErrStateNotFound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this function always returns ErrStateNotFound
if it makes it past the initial s == nil
check. I'd probably invert the if
statement to if !ok
to return early and keep the happy path left (in addition to above in ThreadMemberUpdate
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
restapi.go
Outdated
// ThreadMembers lists the members of a thread | ||
// GET /channels/{channel.id}/threads-members | ||
func (s *Session) ThreadMembers(channelID string) (st []*ThreadMember, err error) { | ||
body, err := s.RequestWithBucketID("GET", EndpointThreadMembers(channelID), nil, EndpointThreadMembers("")) | ||
if err != nil { | ||
return | ||
} | ||
|
||
err = unmarshal(body, &st) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this function works at all:
panic: HTTP 404 Not Found, {"message": "404: Not Found", "code": 0}
It seems like it's calling to /thread-members/
rather than /thread-members
. The fact that EndpointThreadMembers
doesn't take in the user ID as a parameter seems like a bit of a smell. Having two separate endpoint functions and taking in the user ID as a parameter in one (and consequently updating JoinThread
, AddUserToThread
, etc. might be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that things are nearing ready to go here! I noticed that we seem to be limiting many of the thread API calls too strictly by not taking into account the channel ID as a major parameter, but otherwise things are looking great.
// | ||
// POST /channels/{channel.id}/messages/{message.id}/threads | ||
func (s *Session) StartThreadWithMessage(channelID, messageID string, data *ThreadCreateData) (st *Channel, err error) { | ||
body, err := s.RequestWithBucketID("POST", EndpointChannelMessageThreads(channelID, messageID), data, EndpointChannelMessageThreads("", "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the following passage from the rate limits documentation:
Additionally, rate limits take into account major parameters in the URL. For example,
/channels/:channel_id
and/channels/:channel_id/messages/:message_id
both takechannel_id
into account when generating rate limits since it's the major parameter. Currently, the only major parameters arechannel_id
,guild_id
, andwebhook_id + webhook_token
.
It seems to me like the bucket ID should include the channel ID here, since each different channel will be in a different bucket. I think that this also applies to StartThreadWithoutMessage
, JoinThread
, AddUserToThread
, LeaveThread
, RemoveUserFromThread
, ThreadMembers
, PublicArchivedThreads
, PrivateArchivedThreads
, and JoinedPrivateArchivedThreads
(ThreadEditComplex
and ListActiveThreads
already do this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had another chance to look more closely at the code here, and I am really concerned about what I see. It appears that much of the gateway handling code that was introduced in this commit is copied from #982's implementation (it was committed on July 29th there and August 2nd here). I listed three examples in this review; it seems like you attempted to obfuscate this by removing comments and changing !=
to >
, but the code retains almost the exact same structure, the same logic, and the same bugs as the implementation in #982. Are you claiming that the work in this PR is your own? If you were looking to credit the author of #982, the time was when you added that code, not after (and I'm still not sure that this would be an acceptable route given another active PR, but at least it's not direct stealing). I hope that I'm somehow misinterpreting the situation here, but taking code from others' active PRs and trying to represent it as your own is in no way acceptable behavior.
state.go
Outdated
// ThreadMembersUpdate updates the member object of the thread. | ||
// If the user was removed, the ThreadMember object in the state | ||
// will be set to nil. | ||
func (s *State) ThreadMembersUpdate(t *ThreadMembersUpdate) error { | ||
if s == nil { | ||
return ErrNilState | ||
} | ||
|
||
s.Lock() | ||
defer s.Unlock() | ||
|
||
thread, ok := s.channelMap[t.ID] | ||
if ok { | ||
for _, memberID := range t.RemovedMembersIDs { | ||
if memberID == s.User.ID { | ||
thread.Member = nil | ||
break | ||
} | ||
} | ||
} | ||
|
||
return ErrStateNotFound | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR582-R603, and retains the same issue of always returning ErrStateNotFound
.
state.go
Outdated
// ThreadMemberUpdate updates the ThreadMember object for the user, | ||
// in a specific thread. | ||
func (s *State) ThreadMemberUpdate(t *ThreadMember) error { | ||
if s == nil { | ||
return ErrNilState | ||
} | ||
|
||
channel, ok := s.channelMap[t.ID] | ||
if ok { | ||
s.Lock() | ||
defer s.Unlock() | ||
|
||
channel.Member = t | ||
return nil | ||
} | ||
|
||
return ErrStateNotFound | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR564-R580, and retains the same bug of accessing s.channelMap
without any synchronization.
state.go
Outdated
// ThreadListSync syncs new threads and threads member that | ||
// the bot gained access to. | ||
// https://discord.com/developers/docs/topics/gateway#thread-list-sync | ||
func (s *State) ThreadListSync(t *ThreadListSync) error { | ||
if s == nil { | ||
return ErrNilState | ||
} | ||
|
||
g, err := s.Guild(t.GuildID) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if len(t.ChannelIDs) > 0 { | ||
s.Lock() | ||
defer s.Unlock() | ||
|
||
for _, parentChannelID := range t.ChannelIDs { | ||
for _, stateChannel := range s.channelMap { | ||
if stateChannel.IsThread() && stateChannel.ParentID == parentChannelID && !stateChannel.ThreadMetadata.Archived { | ||
for i, c := range g.Channels { | ||
if c.ID == stateChannel.ID { | ||
g.Channels = append(g.Channels[:i], g.Channels[i+1:]...) | ||
break | ||
} | ||
} | ||
delete(s.channelMap, stateChannel.ID) | ||
} | ||
} | ||
} | ||
|
||
for _, channel := range t.Threads { | ||
s.channelMap[channel.ID] = channel | ||
g.Channels = append(g.Channels, channel) | ||
} | ||
|
||
for _, member := range t.Members { | ||
channel, ok := s.channelMap[member.ID] | ||
if ok { | ||
channel.Member = member | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
return ErrStateNotFound | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be copied from https://github.com/bwmarrin/discordgo/pull/982/files/ecd5d0d76e298de23c533d7e4f1f8c026930fb07#diff-320959764bfa2277e0bdd4eaa741f02891d123c42150d4ccb76de0bdbf6e591aR605-R659, and retains the same very unusual structure of the nested loops (it's so non-straightforward that I cannot imagine two people coming to this independently), the same misunderstanding of what the case of len(t.ChannelIDs) == 0
means, and the same return of ErrStateNotFound
when that error really doesn't make a ton of sense in this context.
It is definitely not a copied work. This PR was opened way before that other one. I did take a look at that, to see if I missed something. And I missed the state work and the new webhook type. I also should have mentioned that I added these changes because I saw that PR. I apologize for that, it was my bad. There are bugs there that are not on this PR. And vice versa. A lot of code there is extremely similar to code here too. |
I'm not sure that "we had similar approaches" really does a good job of explaining the situation here. Other than changing one-line map-access-in-
Sure. I'm talking specifically about the state management code, which was added after that one's, as you acknowledge.
Not in the state management code.
Sure, but I'm only referring to the state management code.
I fail to see anything significant that isn't either a natural way of describing things or from Discord's documentation. I unfortunately see no path forward here. I cannot in good conscience accept code that began its journey as copied code, even if it is no longer directly in the final PR. It would be crediting you with someone else's work. I'd recommend really trying to reflect on this and the impact it might have on others if you're planning on contributing to other open-source projects in the future. |
I've started the work on threads.
This PR also adds missing message types/flags.
Any feedback/help is greatly appreciated. Mainly what is missing are the endpoints and the RESTAPI funcs.
-- EDIT
Everything should be done now