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

moderation: refactor clean command #1669

Merged

Conversation

jo3-l
Copy link
Contributor

@jo3-l jo3-l commented Jun 18, 2024

The current implementation of the clean command is somewhat difficult to follow: depending on the arguments, we set about 10 different variables and pass them to AdvancedDeleteMessages as arguments:

numDeleted, err := AdvancedDeleteMessages(parsed.GuildData.GS.ID, parsed.ChannelID, triggerID, userFilter, re, invertRegexMatch, toID, fromID, ma, minAge, pe, attachments, num, limitFetch)

(AdvancedDeleteMessages subsequently checks each filter one-by-one in a long series of conditionals, but this code is not particularly problematic; it is just long, but is relatively easy to understand.)

This PR refactors the code by introducing a Filter interface (which, given a message, reports whether it passes the filter); each filter is a separate struct implementing this interface. The main clean code then boils down to building a slice of Filters based on the arguments, then deleting messages that match all filters. The benefits of this refactor are as follows:

  • Implementations of different filters are isolated (and indeed can now be unit-tested if so desired.) Naturally, it is much easier to find the implementation of a given filter; for instance, if one wishes to verify that the pinned message filter works correctly, we can examine

     if parsed.Switches["nopin"].Bool() {
     	pinned, err := common.BotSession.ChannelMessagesPinned(parsed.ChannelID)
     	if err != nil {
     		return "Failed fetching pinned messages", err
     	}
     	filters = append(filters, NewIgnorePinnedMessagesFilter(pinned))
     }

    and then use go-to-definition on NewIgnorePinnedMessagesFilter to find

     func NewIgnorePinnedMessagesFilter(pinned []*discordgo.Message) *IgnorePinnedMessagesFilter {
     	ids := make(map[int64]struct{})
     	for _, msg := range pinned {
     		ids[msg.ID] = struct{}{}
     	}
     	return &IgnorePinnedMessagesFilter{ids}
     }
    
     func (f *IgnorePinnedMessagesFilter) Matches(msg *dstate.MessageState) (delete bool) {
     	if _, pinned := f.PinnedMsgIDs[msg.ID]; pinned {
     		return false
     	}
     	return true
     }

    whereas previously, one would have to see that the pe variable is set to true if the nopin flag is provided (and then trace how that variable is used in AdvancedDeleteMessages.)

  • The separate AdvancedDeleteMessages function (the one with 10 arguments) can be removed entirely, as the main deletion logic is so simple:

     var toDelete []int64
     filter := CombinedANDFilter{filters} // all filters need to match for message to be deleted
     for _, msg := range msgs {
     	// Can only bulk delete messages up to 2 weeks old (but add 1 minute buffer to be safe.)
     	if now.Sub(msg.ParsedCreatedAt) > (14*time.Hour*24)-time.Minute {
     		continue
     	}
     	// Don't delete the trigger message.
     	if msg.ID == triggerID {
     		continue
     	}
    
     	if filter.Matches(msg) {
     		toDelete = append(toDelete, msg.ID)
     		if len(toDelete) >= deleteLimit {
     			break
     		}
     	}
     }
     ```
  • Still just as easy (arguably slightly more so) to extend with new filters: it suffices to implement a new Filter, then change the clean command to recognize it. Previously one would have had to add a new parameter to AdvancedDeleteMessages, add a new conditional there, and make clean pass that variable to AdvancedDeleteMessages.

  • Despite the additional (and useful) abstraction, the overall diff remains in favor of this change: it is not heavy code-wise.

@ashishjh-bst ashishjh-bst merged commit 792ab09 into botlabs-gg:dev Jun 19, 2024
1 check passed
ashishjh-bst pushed a commit to ashishjh-bst/yagpdb that referenced this pull request Jun 20, 2024
ashishjh-bst pushed a commit to ashishjh-bst/yagpdb that referenced this pull request Jun 27, 2024
ashishjh-bst pushed a commit to ashishjh-bst/yagpdb that referenced this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants