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

[commands] Give all checks a custom exception #2101

Closed
wants to merge 6 commits into from

Conversation

@Vexs
Copy link
Contributor

commented Apr 19, 2019

Summary

This PR adds custom exception classes to all existing commands.
This enables easier use of the existing default checks for more complex error handling.

Role-specific checks now raise NoPrivateMessage instead of just returning False when used outside of a guild as well.

This PR also implements is_SFW, to complement is_NSFW. I don't know what you would use this for, but there it is.

This is technically a breaking change, as users could check very specifically what is raised, however, the impact should be minimal.

Exceptions added to each check:

  • has_role
    • MissingRole
    • NoPrivateMessage
  • has_any_role
    • MissingAnyRole
    • NoPrivateMessage
  • bot_has_role
    • BotMissingRole
    • NoPrivateMessage
  • bot_has_any_role
    • BotMissingAnyRole
    • NoPrivateMessage
  • is_NSFW
    • IsSFW
  • is_SFW
    • IsNSFW

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

Vexs added some commits Apr 10, 2019

Add custom exception classes for checks
Added:
    MissingRole
    BotMissingRole
    MissingAnyRole
    BotMissingAnyRole
    IsSFW
Minor typo/wording adjustments to existing error classes
Merge branch 'master' into all_checks_custom_exceptions
# Conflicts:
#	discord/ext/commands/errors.py
Make checks raise custom error classes
Correct classes to not take in role objects, but instead ints/strs
Add is_sfw check
Wording adjustment
.format is not in-place
@Rapptz
Copy link
Owner

left a comment

Many issues with this implementation:

  1. You are deriving from CommandError instead of CheckFailure. Since this is the case then this does make it a breaking change, since previous code no longer works. You even document that these derive from CheckFailure but do not actually do so.
  2. You need a line in the documentation of each of these classes that it derives from CheckFailure. Something like: "This inherits from :exc:`.CheckFailure`."
  3. You need to update the exception hierarchy in the documentation under api.rst
  4. You need to update the exception listing in the documentation under api.rst
  5. You are passing __init__(self, arg, *args) when it should just be __init__(self, arg), same for the super() call.
  6. I do not agree with the addition of is_sfw, it is useless and if someone wants it then they can do it themselves. is_nsfw was added because it is common enough to warrant inclusion, the same case cannot be said for is_sfw.

I also do not exactly agree with the exceptions IsSFW and IsNSFW. With the removal of is_sfw it becomes slightly easier to do an exception:

class NSFWChannelRequired(CheckFailure):
    """Exception raised when a channel does not have the required NSFW setting.

    This derives from :exc:`.CheckFailure`.

    Parameters
    -----------
    channel: :class:`discord.abc.GuildChannel`
        The channel that does not have NSFW enabled.
    """
    def __init__(self, channel):
        self.channel = channel
        super().__init__('{} needs to be NSFW for this command to work.'.format(channel))

Alternative names:

  • ChannelRequiresNSFW
  • ChannelIsNotNSFW
  • ChannelMissingNSFW
    ...
Show resolved Hide resolved discord/ext/commands/core.py Outdated
Show resolved Hide resolved discord/ext/commands/errors.py
Show resolved Hide resolved discord/ext/commands/errors.py Outdated
Show resolved Hide resolved discord/ext/commands/errors.py
Show resolved Hide resolved discord/ext/commands/errors.py Outdated
Show resolved Hide resolved discord/ext/commands/errors.py Outdated
Show resolved Hide resolved discord/ext/commands/errors.py Outdated
Apply suggestions from code review.
Remove is_sfw and corresponding error
Add versionchanged and versionadded to docstrings
Move has_permissions above bot_has_permissions
Add PrivateMessageOnly to exception hierarchy and listing

@Rapptz Rapptz added this to the v1.1 milestone Apr 20, 2019

@Rapptz Rapptz added the rebased label Apr 20, 2019

@Rapptz

Rapptz approved these changes Apr 20, 2019

@Rapptz Rapptz closed this Apr 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.