CAPR-21: Port purge_cog from deprecated repo - #68
Closed
YC-5002 wants to merge 2 commits into
Closed
Conversation
Contributor
Reviewer's GuidePorts a new Discord purge cog that supports deleting messages by count, duration, or specific date via both slash-command parameters and an interactive UI, and makes a small type-bound adjustment to CallbackModal in the UI module. Sequence diagram for the new purge command interactionssequenceDiagram
actor User
participant DiscordClient
participant PurgeCog
participant PurgeModeView
participant DateTimeModal
participant TextChannel
User->>DiscordClient: invoke slash command purge(amount, duration)
DiscordClient->>PurgeCog: purge(interaction, amount, duration)
alt amount provided
PurgeCog->>TextChannel: purge(limit=amount)
TextChannel-->>PurgeCog: deleted_messages
PurgeCog-->>DiscordClient: followup.send(success message)
DiscordClient-->>User: ephemeral success message
else duration provided
PurgeCog->>PurgeCog: parse_duration(duration)
PurgeCog->>TextChannel: purge(after=now-td)
TextChannel-->>PurgeCog: deleted_messages
PurgeCog-->>DiscordClient: followup.send(success message)
DiscordClient-->>User: ephemeral success message
else interactive mode (no params)
PurgeCog->>PurgeModeView: create view
PurgeCog->>DiscordClient: response.send_message(Select purge mode, view)
DiscordClient-->>User: ephemeral view with mode_select
User->>PurgeModeView: select mode (count | duration | date)
PurgeModeView->>PurgeModeView: on_mode_selected(interaction)
alt mode is count
PurgeModeView->>DiscordClient: response.send_modal(Enter Count)
User->>DiscordClient: submit modal
DiscordClient->>PurgeModeView: _on_submit
PurgeModeView->>PurgeModeView: set value(int)
else mode is duration
PurgeModeView->>DiscordClient: response.send_modal(Enter Duration)
User->>DiscordClient: submit modal
DiscordClient->>PurgeModeView: _on_submit
PurgeModeView->>PurgeModeView: set value(str)
else mode is date
PurgeModeView->>DateTimeModal: create modal
PurgeModeView->>DiscordClient: response.send_modal(DateTimeModal)
User->>DiscordClient: submit modal
DiscordClient->>DateTimeModal: on_submit
DateTimeModal->>PurgeModeView: set value(datetime)
end
PurgeModeView->>PurgeModeView: stop view
PurgeCog->>PurgeCog: _execute_purge(view, channel)
alt view.mode is count
PurgeCog->>TextChannel: purge(limit=value)
else view.mode is duration
PurgeCog->>PurgeCog: parse_duration(value)
PurgeCog->>TextChannel: purge(after=now-td)
else view.mode is date
PurgeCog->>TextChannel: purge(after=date)
end
TextChannel-->>PurgeCog: deleted_messages
PurgeCog-->>DiscordClient: followup.send(result message)
DiscordClient-->>User: ephemeral result
end
Updated class diagram for purge cog and related UI componentsclassDiagram
class Modal
class View
class Cog
class BaseModal
class DateTimeModal {
+DateTimeModal()
}
class PurgeModeView {
+str mode
+int_or_str_or_datetime value
+mode_select
+PurgeModeView()
+on_mode_selected(interaction)
+_prompt_count(interaction)
+_prompt_duration(interaction)
+_prompt_date(interaction)
}
class PurgeCog {
+bot
+logger
+PurgeCog(bot)
+parse_duration(duration)
+_handle_purge_count(amount, channel)
+_handle_purge_duration(duration, channel)
+_handle_purge_date(date, channel)
+purge(interaction, amount, duration)
+_execute_purge(view, channel)
}
class CallbackModal~T~ {
+CallbackModal(title, timeout)
}
Modal <|-- DateTimeModal
View <|-- PurgeModeView
Cog <|-- PurgeCog
BaseModal <|-- CallbackModal
class T {
}
CallbackModal ..|> T
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The change to
CallbackModal's generic definition (class CallbackModal[T: "CallbackModal"]) looks incorrect/recursive compared to the originalTypeVar("T", bound="CallbackModal")and may confuse or break type checking; consider reverting or explicitly defining the generic parameters in a PEP 695–compatible way. - The
durationargument help text ("1h 30m") conflicts withparse_duration, which only accepts a compact1d2h3m-style string without spaces; either adjust the parser to handle spaces or update the user-facing examples to match the actual accepted format. - In the interactive purge path you always prefix the followup message with
"Success "even whensuccessisFalse, and you ignore thesuccessflag in the directamount/durationbranch; consider using the flag to choose different messaging for success vs. failure and avoid the misleading prefix.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `CallbackModal`'s generic definition (`class CallbackModal[T: "CallbackModal"]`) looks incorrect/recursive compared to the original `TypeVar("T", bound="CallbackModal")` and may confuse or break type checking; consider reverting or explicitly defining the generic parameters in a PEP 695–compatible way.
- The `duration` argument help text (`"1h 30m"`) conflicts with `parse_duration`, which only accepts a compact `1d2h3m`-style string without spaces; either adjust the parser to handle spaces or update the user-facing examples to match the actual accepted format.
- In the interactive purge path you always prefix the followup message with `"Success "` even when `success` is `False`, and you ignore the `success` flag in the direct `amount`/`duration` branch; consider using the flag to choose different messaging for success vs. failure and avoid the misleading prefix.
## Individual Comments
### Comment 1
<location> `capy_discord/ui/modal.py:25` </location>
<code_context>
-class CallbackModal[T](BaseModal):
+class CallbackModal[T: "CallbackModal"](BaseModal):
"""A modal that delegates submission logic to a callback function.
</code_context>
<issue_to_address>
**issue (bug_risk):** The generic type parameter declaration here mixes two different typing styles and is likely invalid.
`T` is first defined as `TypeVar("T", bound="CallbackModal")`, then redefined as a PEP 695-style parameter in `class CallbackModal[T: "CallbackModal"](BaseModal):`. This will confuse both the runtime and type checkers. Use one approach consistently:
- Keep `T = TypeVar(...)` and write `class CallbackModal(Generic[T], BaseModal):`, or
- Remove the standalone `TypeVar` and rely solely on the PEP 695 form.
Avoid mixing the two in the same class definition.
</issue_to_address>
### Comment 2
<location> `capy_discord/exts/tools/purge.py:153` </location>
<code_context>
+ if not duration:
+ return None
+
+ pattern = r"(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?"
+ match = re.match(pattern, duration)
+ if not match or not any(match.groups()):
+ return None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The duration regex is not anchored and may accept unintended partial matches.
Since the pattern lacks `^` and `$`, `re.match` will accept valid prefixes of invalid strings (e.g. `"1d2x"` is parsed as 1 day, 2 hours, ignoring the trailing `"x"`). To require the whole string to match the `1d2h3m` format, anchor the regex:
```python
pattern = r"^(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?$"
```
This ensures `parse_duration` returns `None` for inputs with extra characters or spacing.
```suggestion
pattern = r"^(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?$"
```
</issue_to_address>
### Comment 3
<location> `capy_discord/exts/tools/purge.py:198` </location>
<code_context>
+ @app_commands.command(name="purge", description="Delete messages")
+ @app_commands.describe(
+ amount="The number of messages to delete (e.g. 10)",
+ duration="The timeframe to delete messages from (e.g. 1h 30m)",
+ )
+ # @app_commands.checks.has_permissions(manage_messages=True)
</code_context>
<issue_to_address>
**issue (bug_risk):** Duration help text suggests a format that `parse_duration` does not actually accept.
`parse_duration` only accepts compact values like `1d2h3m` (no spaces), but the description shows `"1h 30m"`, which will fail with "Invalid duration format". Please either update the description to reflect the actual format (e.g. `"e.g. 1h30m or 2d3h"`) or update `parse_duration` to support optional whitespace.
</issue_to_address>
### Comment 4
<location> `capy_discord/exts/tools/purge.py:244-245` </location>
<code_context>
+ return
+
+ try:
+ success, message = await self._execute_purge(view, interaction.channel)
+ await interaction.followup.send(f"Success {message}", ephemeral=True)
+ if success:
+ self.logger.info(f"{interaction.user} purged messages in {interaction.channel} using {view.mode} mode")
</code_context>
<issue_to_address>
**issue (bug_risk):** Interactive purge path always prefixes messages with "Success" even when the purge fails.
Here `success, message` is used, but the followup always formats the response as a success, even when `success` is False. That produces misleading output for invalid modes/durations where the message is actually an error. Instead, branch on `success` when building the prefix, e.g. using explicit success/failure markers, or send `message` directly and have the handlers include any needed status indicators.
</issue_to_address>
### Comment 5
<location> `capy_discord/exts/tools/purge.py:275` </location>
<code_context>
+
+
+async def setup(bot: commands.Bot) -> None:
+ """Set up the Sync cog."""
+ await bot.add_cog(PurgeCog(bot))
</code_context>
<issue_to_address>
**nitpick (typo):** Setup function docstring mentions "Sync cog" instead of the actual PurgeCog.
Please update the docstring to reference `PurgeCog` so it matches the cog being registered.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
|
|
||
| class CallbackModal[T](BaseModal): | ||
| class CallbackModal[T: "CallbackModal"](BaseModal): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The generic type parameter declaration here mixes two different typing styles and is likely invalid.
T is first defined as TypeVar("T", bound="CallbackModal"), then redefined as a PEP 695-style parameter in class CallbackModal[T: "CallbackModal"](BaseModal):. This will confuse both the runtime and type checkers. Use one approach consistently:
- Keep
T = TypeVar(...)and writeclass CallbackModal(Generic[T], BaseModal):, or - Remove the standalone
TypeVarand rely solely on the PEP 695 form.
Avoid mixing the two in the same class definition.
| if not duration: | ||
| return None | ||
|
|
||
| pattern = r"(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?" |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The duration regex is not anchored and may accept unintended partial matches.
Since the pattern lacks ^ and $, re.match will accept valid prefixes of invalid strings (e.g. "1d2x" is parsed as 1 day, 2 hours, ignoring the trailing "x"). To require the whole string to match the 1d2h3m format, anchor the regex:
pattern = r"^(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?$"This ensures parse_duration returns None for inputs with extra characters or spacing.
Suggested change
| pattern = r"(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?" | |
| pattern = r"^(?:(\d+)d)?(?:(\d+)h)?(?:(\d+)m)?$" |
| @app_commands.command(name="purge", description="Delete messages") | ||
| @app_commands.describe( | ||
| amount="The number of messages to delete (e.g. 10)", | ||
| duration="The timeframe to delete messages from (e.g. 1h 30m)", |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Duration help text suggests a format that parse_duration does not actually accept.
parse_duration only accepts compact values like 1d2h3m (no spaces), but the description shows "1h 30m", which will fail with "Invalid duration format". Please either update the description to reflect the actual format (e.g. "e.g. 1h30m or 2d3h") or update parse_duration to support optional whitespace.
Comment on lines
+244
to
+245
| success, message = await self._execute_purge(view, interaction.channel) | ||
| await interaction.followup.send(f"Success {message}", ephemeral=True) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Interactive purge path always prefixes messages with "Success" even when the purge fails.
Here success, message is used, but the followup always formats the response as a success, even when success is False. That produces misleading output for invalid modes/durations where the message is actually an error. Instead, branch on success when building the prefix, e.g. using explicit success/failure markers, or send message directly and have the handlers include any needed status indicators.
|
|
||
|
|
||
| async def setup(bot: commands.Bot) -> None: | ||
| """Set up the Sync cog.""" |
Contributor
There was a problem hiding this comment.
nitpick (typo): Setup function docstring mentions "Sync cog" instead of the actual PurgeCog.
Please update the docstring to reference PurgeCog so it matches the cog being registered.
Member
|
Critical Standards Violations
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implemented single line command for deleting messages for both amount and duration mode
Summary by Sourcery
Introduce a purge cog providing flexible message deletion options via a slash command and update a modal generic bound.
New Features:
Enhancements: