Improve error handling

[DubbaThony]

Adds graceful handling to:

• failure to download (ie. "this video is copyrighted and removed" kind of error)
• unknown command

Instead of default behavior unknown command will report back or ignore (depending on config, some users may not wish to have bot pick up on every message), and failure to download will report generic failure message, or if enabled in config (default is disabled) more detailed (but potentially user un-friendly) response.

fixes #7

[maxcutlyp]

Hey, sorry I didn't take a look at this earlier. Looks good to me.

With that said, it's been a while since I've properly looked at this code and even longer since I wrote it, and it looks like the signature for on_command_error may have changed (or maybe it was wrong when I wrote it). According to the docs, the first argument is always a discord.ext.commands.context.Context and the second argument is always a discord.ext.commands.errors.CommandError), which is significantly different from the event: str, *args, **kwargs signature that's there currently. This presumably hasn't caused any issues so far because Python's type hinting is aesthetic only.

The reason I bring this up is that changing the signature to match the docs could simplify the implementation by essentially removing the # test that the exception makes sense section (lines 182-190). So unless this event is triggered at some point with different arguments (or I'm misunderstanding the docs), I think it makes sense to correct the signature and trust that the arguments are correct.

What do you think?

[DubbaThony]

Hi, Im sorry for very late response, I've must have missed notification.

Yes, what you say makes sense to me, I can remove it. Its that Im fairly unfamiliar with the environment and whenever in doubt I just tend to do super defensive programming. I will update the PR to remove the check and trust the lib.

[DubbaThony]

From another things that crossed my mind would be sending ping to user along with error message, for example:

DubbaThony:   .playy https://youtube......
BOT:  @DubbaThony Command not recognized. To see available commands type .help

I think I could implement it. What do you think?

[maxcutlyp]

Nice, I like the update.

I also like the idea of sending the user an error message so that they know what went wrong, although I'm not sure that pinging them is necessary. Only one person in a server would actually be able to use the bot at a time (since it can only be in one VC at once), and as a user of the bot you'd be watching for a reply when you send a command anyway, so I think that pinging the sender wouldn't be necessary. With that said, I'm open to discussion on this.

[DubbaThony]

Hmm, that's fair. Yeah, let's drop that idea.

[maxcutlyp]

Cool. I still think sending an error message is a good idea - something like:

user: .playy https://youtu.be/dQw... bot: command not recognised: .playy. try .help for available commands.

(Example in lowercase to keep on theme with the other lowercase user-facing messages. Obviously feel free to play around with the wording).

Let me know if that's something you're interested in working on, otherwise I'm happy to merge the PR as it is now.

[DubbaThony]

It is implemented already in this PR on line 186, so I would call it good (unless you want to change labels) to merge. The response is capitalized as it's commonly expected by people to have sentences capitalized :)

EDIT: hold on, I see now that I have mixed capitalization, I will normalize it

EDIT2: done, if you prefer lower case LMK, I will change all messages to lower case

[maxcutlyp]

Oh sorry! Not sure how I missed that.

When I mentioned the message being in lowercase, I was more referring to it as a stylistic choice. I'm not sure why I chose to do it like that when I wrote this a few years ago, but I think changing the casing of all the other messages is out of the scope of this PR.

I think the move will be to have all user-facing messages in lowercase (at least for now - I might change that later), including the new "command not recognised" message. Once that's done I'll go ahead and merge it. Thanks for your work on this! (and for your patience with me :p)

[DubbaThony]

No worries. I've normalized all messages to lower case. Note that one message was in upper case before.

Also I've extended vc label to voice channel, as discord now is mainstream enough that plenty of newcomers have no idea what this means and I've seen some to try to parse it as venture capital.

[maxcutlyp]

Good call, and good pickup on the other message. Thanks again!