-
-
Notifications
You must be signed in to change notification settings - Fork 41
add different messages for different types of harmful commands #796
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
Conversation
Reviewer's Guide by SourceryThis pull request modifies the way harmful commands are handled. Instead of a single warning message, specific messages are now displayed based on the type of harmful command detected. The Sequence diagram for handling harmful commandssequenceDiagram
participant User
participant Discord Bot
participant is_harmful Function
User->>Discord Bot: Sends a message containing a command
Discord Bot->>is_harmful Function: is_harmful(message.content)
alt Command is RM_COMMAND
is_harmful Function-->>Discord Bot: Returns "RM_COMMAND"
Discord Bot->>User: Replies with RM_COMMAND warning
else Command is FORK_BOMB
is_harmful Function-->>Discord Bot: Returns "FORK_BOMB"
Discord Bot->>User: Replies with FORK_BOMB warning
else Command is DD_COMMAND
is_harmful Function-->>Discord Bot: Returns "DD_COMMAND"
Discord Bot->>User: Replies with DD_COMMAND warning
else Command is FORMAT_COMMAND
is_harmful Function-->>Discord Bot: Returns "FORMAT_COMMAND"
Discord Bot->>User: Replies with FORMAT_COMMAND warning
else Command is not harmful
is_harmful Function-->>Discord Bot: Returns None
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @amyulating - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a dictionary or a match statement to map the harmful command type to the appropriate message.
- It might be helpful to add a default case or error handling for when
is_harmfulreturns None.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
tux/handlers/event.py
Outdated
| stripped_content = strip_formatting(message.content) | ||
|
|
||
| if is_harmful(stripped_content): | ||
| if is_harmful(stripped_content) == "RM_COMMAND": |
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.
suggestion (performance): Avoid multiple evaluations of is_harmful.
The function is_harmful(stripped_content) is currently called repeatedly for each condition. Consider storing its result in a variable (e.g., harmful_type) and then using if/elif statements. This not only improves performance but also enhances code clarity by ensuring the check is only performed once.
Suggested implementation:
stripped_content = strip_formatting(message.content)
harmful_type = is_harmful(stripped_content)
if harmful_type == "RM_COMMAND": elif harmful_type == "FORK_BOMB": elif harmful_type == "DD_COMMAND": elif harmful_type == "FORMAT_COMMAND":
tux/utils/functions.py
Outdated
|
|
||
|
|
||
| def is_harmful(command: str) -> bool: | ||
| def is_harmful(command: str) -> str: |
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.
suggestion: Align return type annotation with possible None return.
The function may return a string (e.g., "RM_COMMAND", "FORK_BOMB", etc.) or None. Updating the signature to use Optional[str] would more accurately reflect its behavior.
Suggested implementation:
def is_harmful(command: str) -> Optional[str]:from typing import Optional
fixes #759
Summary by Sourcery
Enhance harmful command detection to provide more specific warning messages for different types of potentially destructive commands
Bug Fixes:
Enhancements: