discord_messages #2

Closed
AsafGartner wants to merge 0 commits from discord_messages into master
Owner
No description provided.
AsafGartner added 3 commits 2022-01-31 06:57:20 +00:00
AsafGartner added 1 commit 2022-01-31 08:22:45 +00:00
bvisness reviewed 2022-02-03 01:44:13 +00:00
bvisness left a comment
Owner

Looking good overall! I'm glad you introduced the concept of an "interned message"; I think that helps a lot with our communication about this topic at least.

I obviously left a lot of comments but I don't see anything fundamentally wrong here. I think there might still be some edge cases to consider with message editing and how that propagates to snippets and interned data, though. Maybe we should talk about that synchronously and figure out if it's even worth it to try and synchronize things like that.

Looking good overall! I'm glad you introduced the concept of an "interned message"; I think that helps a lot with our communication about this topic at least. I obviously left a lot of comments but I don't see anything fundamentally wrong here. I think there might still be some edge cases to consider with message editing and how that propagates to snippets and interned data, though. Maybe we should talk about that synchronously and figure out if it's even worth it to try and synchronize things like that.
@ -38,3 +41,3 @@
Use: "makesnippet [<message id>...]",
Use: "makesnippet <channel id> [<message id>...]",
Short: "Make snippets from saved Discord messages",
Long: "Make snippets from Discord messages whose content we have already saved. Useful for creating snippets from messages in non-showcase channels.",
Owner

These descriptions are no longer correct.

These descriptions are no longer correct.
AsafGartner marked this conversation as resolved
@ -615,2 +597,4 @@
}
// NOTE(asaf): Since any error from HandleIncomingMessage is an internal error and not a discord
// error, we only want to log it and not restart the bot. So we're not returning the error.
Owner

tab alignment :sadge:

tab alignment :sadge:
AsafGartner marked this conversation as resolved
@ -677,3 +612,1 @@
`,
result.SnippetID,
)
if interned != nil {
Owner

It may not be that important, but I usually find that when I return nil, nil from a function, I end up forgetting the nil check and causing bugs.

Basically, I don't think pointers in Go are a great way to represent an optional value as an API decision. There are lots of reasons to return a pointer even when a non-nil value is guaranteed, and it's bitten me so many times that I have decided to just always use errors for that. That's why I've been returning db.NotFound from the post helper functions and such. I'm way more likely to handle things correctly that way.

I'm open to discussion about how we want to handle this, but I also would probably like this to be consistent with the rest of the codebase, especially as we move toward adding more contributors. So I'd like to get your thoughts on that and come to a consensus.

It may not be that important, but I usually find that when I return `nil, nil` from a function, I end up forgetting the nil check and causing bugs. Basically, I don't think pointers in Go are a great way to represent an optional value as an API decision. There are lots of reasons to return a pointer even when a non-nil value is guaranteed, and it's bitten me so many times that I have decided to just always use errors for that. That's why I've been returning `db.NotFound` from the post helper functions and such. I'm way more likely to handle things correctly that way. I'm open to discussion about how we want to handle this, but I also would probably like this to be consistent with the rest of the codebase, especially as we move toward adding more contributors. So I'd like to get your thoughts on that and come to a consensus.
AsafGartner marked this conversation as resolved
@ -38,0 +36,4 @@
// NOTE(asaf): 5 seconds to ensure this runs on start, we then reset it to the correct
// interval after the first run.
backfillTicker := time.NewTicker(5 * time.Second)
Owner

If we're going to be manually resetting the ticker after every tick, it might as well not be a ticker.

I would probably change this to be a time.Timer, or perhaps even make a little utility called DelayedTicker(delay time.Duration, duration time.Duration) that we can use for cases like this. A utility might be overkill, but it also would be very self-contained, and I would trust it more because of how likely it is that we would fail to timer.Reset on some branch.

If we're going to be manually resetting the ticker after every tick, it might as well not be a ticker. I would probably change this to be a `time.Timer`, or perhaps even make a little utility called `DelayedTicker(delay time.Duration, duration time.Duration)` that we can use for cases like this. A utility might be overkill, but it also would be very self-contained, and I would trust it more because of how likely it is that we would fail to `timer.Reset` on some branch.
AsafGartner marked this conversation as resolved
@ -47,1 +49,4 @@
case <-backfillTicker.C:
backfillTicker.Reset(backfillInterval)
// TODO(asaf): Do we need to update lastBackfillTime here? Otherwise we'll be rescraping
// from (start up time - 1 hour) every time.
Owner

Yeah this clearly seems like a bug. Seems like we should be setting this, or perhaps even storing this value in the database...let's just set it here for now though.

I will say though, it seems weird to just assume that we should always go back one backfill interval when we boot up the site. I guess it assumes that the Discord bot would be down for less than the backfill interval, which is probably true, but it seems a bit wrong to correlate the two. I would probably opt to change line 41 to something like this:

lastBackfillTime := time.Now().Add(-3 * time.Hour) // or however long of an outage we want to tolerate
Yeah this clearly seems like a bug. Seems like we should be setting this, or perhaps even storing this value in the database...let's just set it here for now though. I will say though, it seems weird to just assume that we should always go back one backfill interval when we boot up the site. I guess it assumes that the Discord bot would be down for less than the backfill interval, which is probably true, but it seems a bit wrong to correlate the two. I would probably opt to change line 41 to something like this: ```go lastBackfillTime := time.Now().Add(-3 * time.Hour) // or however long of an outage we want to tolerate ```
AsafGartner marked this conversation as resolved
@ -109,0 +111,4 @@
continue
}
if interned == nil {
log.Error().Msg("couldn't find interned message")
Owner

Is it actually an error to not find an interned message in our db? I would expect that to be relatively likely, although I guess the live websocket connection would probably do that before a scrape.

Either way, since you don't have an error to attach to this, I would probably deem this as only worthy of an .Info() or at most a .Warn().

Is it actually an error to not find an interned message in our db? I would expect that to be relatively likely, although I guess the live websocket connection would probably do that before a scrape. Either way, since you don't have an `error` to attach to this, I would probably deem this as only worthy of an `.Info()` or at most a `.Warn()`.
Author
Owner

It's kind of an error here because we just pulled that same message out of the db.

Step 1: Get a list of interned message IDs that don't have content.
Step 2: Get the full interned message struct for each one.

It doesn't make much sense to find a message in step 1 and then not find it in step 2.

It's kind of an error here because we just pulled that same message out of the db. Step 1: Get a list of interned message IDs that don't have content. Step 2: Get the full interned message struct for each one. It doesn't make much sense to find a message in step 1 and then not find it in step 2.
Owner

Gotcha, that makes sense - let's at least log the message ID in this case though.

Gotcha, that makes sense - let's at least log the message ID in this case though.
AsafGartner marked this conversation as resolved
@ -0,0 +38,4 @@
if !deleted && err == nil {
err = MaybeInternMessage(ctx, dbConn, msg)
}
Owner

If we think we might add more to this chain in the future, maybe we should add a setup like

type messageHandlerFunc func(ctx context.Context, dbConn db.ConnOrTx, msg *Message) (deleted bool, err error)

var handlers := []messageHandlerFunc{CleanUpLibrary, CleanUpShowcase, MaybeInternMessage}

// and then loop over them or whatever

but even as I type that it feels probably premature, and it would be hard to pass other arguments to and whatnot.

If we think we might add more to this chain in the future, maybe we should add a setup like ```go type messageHandlerFunc func(ctx context.Context, dbConn db.ConnOrTx, msg *Message) (deleted bool, err error) var handlers := []messageHandlerFunc{CleanUpLibrary, CleanUpShowcase, MaybeInternMessage} // and then loop over them or whatever ``` but even as I type that it feels probably premature, and it would be hard to pass other arguments to and whatnot.
Author
Owner

I think it's simple enough for now.

I think it's simple enough for now.
@ -0,0 +49,4 @@
func CleanUpShowcase(ctx context.Context, dbConn db.ConnOrTx, msg *Message) (bool, error) {
deleted := false
if msg.ChannelID == config.Config.Discord.ShowcaseChannelID {
Owner

This all might be easier with an early return, if you feel like it.

This all might be easier with an early return, if you feel like it.
@ -0,0 +51,4 @@
deleted := false
if msg.ChannelID == config.Config.Discord.ShowcaseChannelID {
switch msg.Type {
case MessageTypeDefault, MessageTypeReply, MessageTypeApplicationCommand:
Owner

Do we want to include application commands here? Seems maybe wrong. We probably won't be able to respond inline in the channel without the response getting nuked...

Do we want to include application commands here? Seems maybe wrong. We probably won't be able to respond inline in the channel without the response getting nuked...
Author
Owner

No idea. I copied the cleanup code as-is. We should test this.

No idea. I copied the cleanup code as-is. We should test this.
@ -0,0 +56,4 @@
return deleted, nil
}
hasGoodContent := true
Owner

Why does this start out true? Is this what makes application commands work?

Why does this start out true? Is this what makes application commands work?
@ -0,0 +99,4 @@
deleted := false
if msg.ChannelID == config.Config.Discord.LibraryChannelID {
switch msg.Type {
case MessageTypeDefault, MessageTypeReply, MessageTypeApplicationCommand:
Owner

Same concerns about application commands and early returns apply here.

Same concerns about application commands and early returns apply here.
@ -0,0 +157,4 @@
the database.
This does not create snippets or save content or do anything besides save the message itself.
*/
Owner

Comment is clearly in the wrong place - I'm guessing also outdated?

Comment is clearly in the wrong place - I'm guessing also outdated?
AsafGartner marked this conversation as resolved
@ -0,0 +716,4 @@
if existingSnippet != nil {
// TODO(asaf): We're not handling the case where embeds were removed or modified.
// Also not handling the case where a message had both an attachment and an embed
// and the attachment was removed (leaving only the embed).
Owner

Do you want to handle that in this first pass? Seems like we should do something if the user does something that causes the message to be deleted - but would that cascade through this snippet elsewhere?

Do you want to handle that in this first pass? Seems like we should do something if the user does something that causes the message to be deleted - but would that cascade through this snippet elsewhere?
Author
Owner

It wouldn't cause the message to be deleted. It would just leave the message with different content from what we captured when we initially created the snippet.

It wouldn't cause the message to be deleted. It would just leave the message with different content from what we captured when we initially created the snippet.
@ -0,0 +717,4 @@
// TODO(asaf): We're not handling the case where embeds were removed or modified.
// Also not handling the case where a message had both an attachment and an embed
// and the attachment was removed (leaving only the embed).
LinkedUserIsSnippetOwner := existingSnippet.OwnerID == interned.DiscordUser.HMNUserId
Owner

capital L! reeeeeeee

capital L! reeeeeeee
AsafGartner marked this conversation as resolved
@ -0,0 +789,4 @@
}
if existingSnippet != nil {
// Update tags
Owner

What happens if the user updates a message to remove a tag? Is there any path that will un-tag the snippet? Do we want that?

What happens if the user updates a message to remove a tag? Is there any path that will un-tag the snippet? Do we want that?
Author
Owner

What do you mean? This path deletes all project tags and only re-tags with whatever is in the message. Untagging just works.

What do you mean? This path deletes all project tags and only re-tags with whatever is in the message. Untagging just works.
Owner

Oh, never mind then.

Oh, never mind then.
bvisness marked this conversation as resolved
@ -0,0 +793,4 @@
// Try to associate tags in the message with project tags in HMN.
// Match only tags for projects in which the current user is a collaborator.
messageTags := getDiscordTags(existingSnippet.Description)
Owner

Random thought based on usage I saw on HMN - we need to make sure the tag parser can recognize tags in situations like:

check out this new feature in &WhiteBox's UI

We should have an automated test for that and other situations if we don't yet already.

Random thought based on usage I saw on HMN - we need to make sure the tag parser can recognize tags in situations like: ``` check out this new feature in &WhiteBox's UI ``` We should have an automated test for that and other situations if we don't yet already.
@ -0,0 +878,4 @@
}
// TODO(asaf): I believe this will also match https://example.com?hello=1&whatever=5
// Probably need to add word boundaries.
Owner

Ok, yet more reasons to update and verify this in all kinds of uses. Testing!!!

Ok, yet more reasons to update and verify this in all kinds of uses. Testing!!!
@ -183,0 +172,4 @@
return c.ErrorResponse(http.StatusInternalServerError, err)
}
if interned != nil {
// NOTE(asaf): Creating snippet even if the checkbox is off because the user asked us to.
Owner

We should add some text to the profile settings page informing the user of this.

We should add some text to the profile settings page informing the user of this.
AsafGartner added 1 commit 2022-02-07 12:22:03 +00:00
AsafGartner added 1 commit 2022-02-07 12:25:35 +00:00
Author
Owner

I configured the mailer for gitea. Is this on?

I configured the mailer for gitea. Is this on?
AsafGartner closed this pull request 2022-02-10 18:45:16 +00:00

Pull request closed

Sign in to join this conversation.
No description provided.