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

fix UserInputError being handled incorrectly. #77

Merged
merged 5 commits into from
Jan 23, 2021
Merged

Conversation

vivekashok1221
Copy link
Member

UserInputError has been made part of the if-elif block.
This should fix issue #76.

Additionally, MissingRequiredArgument now shows a different message.
old:
image

now:
image

@vivekashok1221 vivekashok1221 linked an issue Jan 17, 2021 that may be closed by this pull request
@gustavwilliam
Copy link
Member

Thanks. Could you also capitalize the description, so it doesn’t start with a lowercase letter?

Copy link
Contributor

@uncomfyhalomacro uncomfyhalomacro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gustavwilliam you might want to look at this. @codephile1221 Thanks for noticing the bug

f"Your input was invalid: {error}\n\nUsage:\n"
f"```{ctx.prefix}{ctx.command} {ctx.command.signature}```"
)
if isinstance(error, commands.MissingRequiredArgument):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if there is a MissingRequiredArgument happens, this bug occurs? nice catch 💯

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if there is a MissingRequiredArgument happens, this bug occurs? nice catch 100

Actually, this bug occurs if any UserInputError happens (including MissingRequriedArgument).

I added special handling of MissingRequiredArgument just so that the message is different.

Copy link
Contributor

@Shivansh-007 Shivansh-007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks Good To Me!

In future we could do something like this:

def error_handler():
  if userinputerror:
    handle_user_input_error()
  elif checkserror:
   handle_checkst_error():
  ... others

def handle_user_input_error():
        """
        * MissingRequiredArgument
        * TooManyArguments
        * BadArgument
        * ArgumentParsingError
        """

def handle_checkst_error():
        """
        * BotMissingPermissions
        * BotMissingRole
        * NoPrivateMessage
        """

... etc.

@vivekashok1221
Copy link
Member Author

Thanks. Could you also capitalize the description, so it doesn’t start with a lowercase letter?

Yeah, I thought of that. I think rather than capitalizing, formatting the parameter (language in this case) with backticks is more appropriate. So the description would be "language is a missing required argument which is missing."

Or, we could alter the entire sentence structure.

There are two ways this can be carried out.

  1. Get the error in str and format the first word (This is assuming the first word will be the parameter)
  2. I believe MissingRequriedArgument can provide you with the missing argument and we should construct a message accordingly.

Both are easy to implement.

This method is to be removed after A default embed helper function #30
Comment on lines +31 to +32
title = title or random.choice(ERROR_REPLIES)
embed = Embed(colour=Colours.soft_red, title=title)
Copy link
Member Author

@vivekashok1221 vivekashok1221 Jan 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part will be removed anyway after issue #30. However, VS Code was bothering me and I changed it.

@vivekashok1221
Copy link
Member Author

image

Rather than capitalising, I decided to format the missing argument with a single line code-block.
Feel free to suggest changes.

@Shivansh-007
Copy link
Contributor

Yeah that looks better.

Copy link
Member

@gustavwilliam gustavwilliam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Shivansh-007
Copy link
Contributor

Can we merge this as it is a high priority feature?

Copy link
Contributor

@vivax3794 vivax3794 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gustavwilliam
Copy link
Member

it is a high priority feature?

@Shivansh-007 Well, yeah, it's currently one of the most important features on gurkbot, but it's not per say a high priority feature. We have a label for that, and this one does not warrant it.

@gustavwilliam gustavwilliam merged commit e2ee68c into main Jan 23, 2021
@gustavwilliam gustavwilliam deleted the fix/error-handler branch January 23, 2021 12:01
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.

UserInputErrors are handled improperly
5 participants