{"action":"created","issue":{"url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/166","repository_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector","labels_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/166/labels{/name}","comments_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/166/comments","events_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/166/events","html_url":"https://github.com/Charcoal-SE/SmokeDetector/issues/166","id":160677953,"number":166,"title":"Clean up handle_commands discussion","user":{"login":"AWegnerGitHub","id":6462439,"avatar_url":"https://avatars.githubusercontent.com/u/6462439?v=3","gravatar_id":"","url":"https://api.github.com/users/AWegnerGitHub","html_url":"https://github.com/AWegnerGitHub","followers_url":"https://api.github.com/users/AWegnerGitHub/followers","following_url":"https://api.github.com/users/AWegnerGitHub/following{/other_user}","gists_url":"https://api.github.com/users/AWegnerGitHub/gists{/gist_id}","starred_url":"https://api.github.com/users/AWegnerGitHub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AWegnerGitHub/subscriptions","organizations_url":"https://api.github.com/users/AWegnerGitHub/orgs","repos_url":"https://api.github.com/users/AWegnerGitHub/repos","events_url":"https://api.github.com/users/AWegnerGitHub/events{/privacy}","received_events_url":"https://api.github.com/users/AWegnerGitHub/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/labels/enhancement","name":"enhancement","color":"84b6eb"}],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2016-06-16T14:37:34Z","updated_at":"2016-07-13T21:20:11Z","closed_at":"2016-07-13T21:20:11Z","body":"# Summary\r\n\r\nThe [`handle_commands`](https://github.com/Charcoal-SE/SmokeDetector/blob/aaf332daa8588394af555357d2bd1da32966a234/chatcommunicate.py#L170) function is a 500 line monstrosity. I'd like to discuss cleaning it up to do the following:\r\n\r\n- Standardize what is returned from `handle_commands`\r\n- Have individual functions for each command. \r\n \r\nIf we agree that this work is needed, I'll take on the work load over the next several weeks (translation: after my summer vacation) to do this work.\r\n \r\n# Clean up details\r\n\r\nThis section describes my plan for the above changes. I'd like to do both changes at the same time, but if opposition to that exists one change at a time is acceptable to me.\r\n\r\n## Standardize what is returned from `handle_commands`\r\n\r\nCurrently, the function has multiple return paths and multiple ways of returning data. \r\n\r\nOne example is in the [`!!/block`](https://github.com/Charcoal-SE/SmokeDetector/blob/master/chatcommunicate.py#L491) command:\r\n\r\n    return False, \"Invalid duration.\"\r\n\t\r\nAnother example is in [`!!/errorlogs`](https://github.com/Charcoal-SE/SmokeDetector/blob/master/chatcommunicate.py#L513)\r\n\r\n    return \"The !!/errorlogs command requires 1 argument.\"\r\n\t\r\nThese two methods lead to weird handling in the two locations that `handle_commands` is called from. ([Location 1](https://github.com/Charcoal-SE/SmokeDetector/blob/master/chatcommunicate.py#L123), [Location 2](https://github.com/Charcoal-SE/SmokeDetector/blob/master/chatcommunicate.py#L157))\r\n\r\nMethod of handling 1:\r\n\r\n    if type(result) != tuple:\r\n        result = (True, result)\r\n    if result[1] is not None:\r\n        if wrap2.host + str(message_id) in GlobalVars.listen_to_these_if_edited:\r\n            GlobalVars.listen_to_these_if_edited.remove(wrap2.host + str(message_id))\r\n        message_with_reply = u\":{} {}\".format(message_id, result[1])\r\n        if len(message_with_reply) <= 500 or \"\\n\" in result[1]:\r\n            ev.message.reply(result[1], False)\r\n    if result[0] is False:\r\n        add_to_listen_if_edited(wrap2.host, message_id)\r\n\r\nMethod of handling 2:\r\n\r\n    r = result\r\n    if type(result) == tuple:\r\n        result = result[1]\r\n    if result is not None and result is not False:\r\n        reply += result + os.linesep\r\n    elif result is None:\r\n        reply += \"<processed without return value>\" + os.linesep\r\n        amount_none += 1\r\n    elif result is False or r[0] is False:\r\n        reply += \"<unrecognized command>\" + os.linesep\r\n        amount_unrecognized += 1\r\n\t\t\r\nThese type of checks are unintuitive and difficult to follow. It also adds two sections of unnecessary code that perform in a similar, but not identical way. \r\n\r\n### Proposal\r\n\r\nI propose that all return paths in `handle_commands` return the expected tuple of `(boolean, string)` and the two locations that call `handle_commands` do so like this, with appropriate variable names:\r\n\r\n    bool_result, string_result = handle_commands(...)\r\n\t\r\nThe block of code afterward can then be reduced and simplified by using descriptive variables instead of elements of a tuple.\r\n\r\n## Have individual functions for each command.\r\n\r\nAs `handle_commands` is currently written, *all* command processing is occurring in this function. Much of this logic should be moved to their own individual functions and `handle_commands` should be used to determine which function to call. Each block that looks like the following should have their own function:\r\n\r\n    if content_lower.startswith(\"!!/addblu\")\r\n\t\r\n### Proposal\r\n\r\nMove the logic of each command to it's own function. This keeps `handle_commands` cleaner and allows for certain areas of code to be reused "},"comment":{"url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/comments/232490222","html_url":"https://github.com/Charcoal-SE/SmokeDetector/issues/166#issuecomment-232490222","issue_url":"https://api.github.com/repos/Charcoal-SE/SmokeDetector/issues/166","id":232490222,"user":{"login":"AWegnerGitHub","id":6462439,"avatar_url":"https://avatars.githubusercontent.com/u/6462439?v=3","gravatar_id":"","url":"https://api.github.com/users/AWegnerGitHub","html_url":"https://github.com/AWegnerGitHub","followers_url":"https://api.github.com/users/AWegnerGitHub/followers","following_url":"https://api.github.com/users/AWegnerGitHub/following{/other_user}","gists_url":"https://api.github.com/users/AWegnerGitHub/gists{/gist_id}","starred_url":"https://api.github.com/users/AWegnerGitHub/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AWegnerGitHub/subscriptions","organizations_url":"https://api.github.com/users/AWegnerGitHub/orgs","repos_url":"https://api.github.com/users/AWegnerGitHub/repos","events_url":"https://api.github.com/users/AWegnerGitHub/events{/privacy}","received_events_url":"https://api.github.com/users/AWegnerGitHub/received_events","type":"User","site_admin":false},"created_at":"2016-07-13T21:20:10Z","updated_at":"2016-07-13T21:20:10Z","body":"Completed with #168 and #174 "}}
{"actor":{"display_login":"AWegnerGitHub"}}