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

Patch to make !help work outside of #bot-commands again #263

Merged
merged 1 commit into from Jan 9, 2019

Conversation

4 participants
@SebastiaanZ
Copy link
Member

SebastiaanZ commented Jan 9, 2019

This patches !help not working outside of the bot-commands channel. I think a proper rewrite of the procedure may be warranted, but this will do in the mean time. See issue #262

Basically what happened is that the help cog runs all the checks on the commands to see if the user requesting the help-embed is allowed to run it to determine which commands should be included. Apparently, one of those checks raises an exception when the check fails, which stops function execution, preventing the help-embed from showing up.

I'm now catching it with a try-except and saying that the user can't run the command if the exception was raised. That solves the bug, but creates another one that should be solved later (and is less severe, in my opinion):

If a user ask for !help outside of bot-commands, commands that are only allowed in bot-commands do not show up since they user is not allowed to run that command in the channel where they are requesting the help-embed. I don't think that's right, so we should probably restructure the checks.

However, I'd rather push this out first and rewrite the can_run check later.

@sco1 sco1 added this to Needs review in Bot Tracking via automation Jan 9, 2019

# the mean time.
try:
can_run = await command.can_run(self._ctx)
except InChannelCheckFailure:

This comment has been minimized.

@jchristgit

jchristgit Jan 9, 2019

Member

Shouldn't we apply this to all check failures?

This comment has been minimized.

@SebastiaanZ

SebastiaanZ Jan 9, 2019

Member

I think this is the only one that raises an exception to be able to return a custom message to the user. The other checks return True or False, which was the expected return to assign to can_run, I think.

This comment has been minimized.

@SebastiaanZ

SebastiaanZ Jan 9, 2019

Member

But, we may want to think of something in case other checks with exceptions get added.

This comment has been minimized.

@jos-b

jos-b Jan 9, 2019

Member

Why are we using exceptions in the first place if the expected return is a boolean? Shouldn't we be returning booleans like the other checks?

This comment has been minimized.

@SebastiaanZ

SebastiaanZ Jan 9, 2019

Member

Oh, I meant something different:

  • The one who implemented this at this point probably expected a boolean and didn't realize it could be an Exception
  • The reason it's returning an exception is because that allows for a custom Exception message to go along with it. It's used to display a message to user: f"Sorry, but you may only use this command within {channels_str}.

The discord.py API allows for three possible return values, one of the booleans or an exception like this one. I think it was an oversight at this point in the code.

This comment has been minimized.

@jos-b

jos-b Jan 9, 2019

Member

Ah understood

@jos-b

jos-b approved these changes Jan 9, 2019

@eivl

eivl approved these changes Jan 9, 2019

Bot Tracking automation moved this from Needs review to Reviewer approved Jan 9, 2019

@SebastiaanZ SebastiaanZ merged commit fce4c0d into master Jan 9, 2019

Bot Tracking automation moved this from Reviewer approved to Done Jan 9, 2019

@SebastiaanZ SebastiaanZ deleted the help-command-patch branch Jan 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment