Fixed player subcommand, removed debug outputs #41

2016-04-11T15:00:49Z

Pepich commented

2016-04-11 15:00:49 +00:00

(Migrated from github.com)

No description provided.

jomo commented

2016-04-12 12:18:18 +00:00

(Migrated from github.com)

I don't think it's a good idea to remove debug output and catch exceptions, i.e. silently ignoring them. We either need the traces for debugging or – when we're confident there are no more bugs – the try/catch should also be gone. The latter will still print exceptions to the log and tell the command issuer that something went wrong.

Somewhat related, I don't think printing the command help when there's an exception is a good idea at all, it's just confusing.

I don't think it's a good idea to remove debug output **and** catch exceptions, i.e. silently ignoring them. We either need the traces for debugging or – when we're confident there are no more bugs – the try/catch should also be gone. The latter will still print exceptions to the log and tell the command issuer that something went wrong. Somewhat related, I don't think printing the command help when there's an exception is a good idea at all, it's just confusing.

Pepich commented

2016-04-12 12:44:24 +00:00

(Migrated from github.com)

The only two places where it is printing the help screen are lines 87 and 282, both of which are catching the case that the user is calling the command with a missing argument. If you want console to throw an error each time someone types /alias I'm fine with that but I rather would hide it. Adding a test for the length of the argument would resolve that issue yet if something goes wrong and goes to that catch it is directly related to the user not giving proper input and not the plugin or database doing something wrong. The only case where I think we actually would want it back is line 157 (definitly catch it though).

Line 126 the try/catch can just be removed, it's not nessecary anymore.

The only two places where it is printing the help screen are lines 87 and 282, both of which are catching the case that the user is calling the command with a missing argument. If you want console to throw an error each time someone types /alias I'm fine with that but I rather would hide it. Adding a test for the length of the argument would resolve that issue yet if something goes wrong and goes to that catch it is directly related to the user not giving proper input and not the plugin or database doing something wrong. The only case where I think we actually would want it back is line 157 (definitly catch it though). Line 126 the try/catch can just be removed, it's not nessecary anymore.

This repo is archived. You cannot comment on pull requests.

No reviewers