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

Feature/rpc rewrite #324

Closed

Conversation

dennisrijsdijk
Copy link

No description provided.

@dennisrijsdijk dennisrijsdijk marked this pull request as draft October 27, 2023 10:47
Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

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

in general this looks nice

I'm unsure if I like how subscribing to events work.
Maybe it should work more like in the normal bot where you register handlers via AddEventListeners and the subscribe method won't take a handler

I'm open for discussion here

rpc/client.go Outdated Show resolved Hide resolved
rpc/client.go Outdated Show resolved Hide resolved
rpc/client.go Outdated Show resolved Hide resolved
rpc/client.go Outdated Show resolved Hide resolved
rpc/client.go Outdated Show resolved Hide resolved
rpc/cmd.go Outdated Show resolved Hide resolved
rpc/cmd.go Outdated Show resolved Hide resolved
rpc/embed.go Outdated
Comment on lines 1 to 37
package rpc

import (
"time"

"github.com/disgoorg/disgo/discord"
)

// The EmbedResource of an Embed.Image/Embed.Thumbnail/Embed.Video
type EmbedResource struct {
URL string `json:"url,omitempty"`
ProxyURL string `json:"proxyURL,omitempty"`
Height int `json:"height,omitempty"`
Width int `json:"width,omitempty"`
}

type Embed struct {
ID string `json:"id"` // eg. "embed_N"
Type discord.EmbedType `json:"type"` // Only seen "rich" so far
RawTitle string `json:"rawTitle"`
RawDescription string `json:"rawDescription"`
Color string `json:"color"` // CSS color, why discord why.
Fields []discord.EmbedField `json:"fields"`
URL string `json:"url,omitempty"`
Timestamp *time.Time `json:"timestamp,omitempty"`
Footer *discord.EmbedFooter `json:"footer,omitempty"`
Image *EmbedResource `json:"image,omitempty"`
Thumbnail *EmbedResource `json:"thumbnail,omitempty"`
Video *EmbedResource `json:"video,omitempty"`
Provider *discord.EmbedProvider `json:"provider,omitempty"`
Author *discord.EmbedAuthor `json:"author,omitempty"`
}

type Attachment struct {
*discord.Attachment
Spoiler bool `json:"spoiler,omitempty"`
}
Copy link
Member

Choose a reason for hiding this comment

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

is embed so different from discord.Embed?

Copy link
Author

Choose a reason for hiding this comment

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

changed in fb13deb, looking for more feedback.

rpc/message.go Outdated Show resolved Hide resolved
@topi314 topi314 marked this pull request as ready for review October 30, 2023 12:11
Copy link
Member

@topi314 topi314 left a comment

Choose a reason for hiding this comment

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

I think we need to talk about the event subscriptions

return nil
}

type message struct {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep this exported. Kinda annoying that those names clash I agree.

Copy link
Author

Choose a reason for hiding this comment

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

Would you suggest I rename this or Message (as in ChannelMessage)?

// Attachment is used for files sent in a Message
type Attachment struct {
*discord.Attachment
Spoiler bool `json:"spoiler,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

a file is marked as spoiler when the name starts with SPOILER_
so we could perhaps move that into discord.Attachment and work with a custom unmarshaller?

rpc/message.go Outdated Show resolved Hide resolved
)

// The EmbedResource of an Embed.Image/Embed.Thumbnail/Embed.Video
type EmbedResource struct {
Copy link
Member

Choose a reason for hiding this comment

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

this should prob be unexported

// The EmbedResource of an Embed.Image/Embed.Thumbnail/Embed.Video
type EmbedResource struct {
URL string `json:"url,omitempty"`
ProxyURL string `json:"proxyURL,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

wtf discord, you are disgusting

type message Message
var v struct {
message
Timestamp time.Time `json:"timestamp"` // discord.Message.CreatedAt
Copy link
Member

Choose a reason for hiding this comment

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

we already use timestamp in discord.Message here. We just changed the field name compared to what the API delivers

@@ -268,29 +339,147 @@ type CmdArgs interface {
cmdArgs()
}

type Event string
type Event struct {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the purpose of this is

Copy link
Author

Choose a reason for hiding this comment

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

Related atm to keeping track of subscribed events (event name + args) and handlers. to be revisited when we decide on the approach for events going forward.

Co-authored-by: Toπ <git@topi.wtf>
@dennisrijsdijk dennisrijsdijk closed this by deleting the head repository May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants