Refactoring and testing DeltaBot

[chrisuehlinger]

Hey! So I've been talking with PixelOrange about making this project easier to test. Given that many people are coming and going on this project, I think testing would be a great way to make sure new contributors don't accidentally step on old code or break something that was written a long time ago.

If you are currently working on adding a feature, that's fine, but try to follow these principles as much as possible:

-Write pure functions: If given the same input two times, the function should give the same output both times.
-If you have to make calls to praw, do them higher up and then pass the results into your new function.
-Don't log inside of your functions: rather, return a variable containing what you would have logged, along with the result of the function. (this is less important)

Here's a list of the pure and impure functions with their side effects.

Pure:
get_first_int
flair_sorter
skippable_line
str_contains_token
markdown_to_scoreboard
scoreboard_to_markdown
string_matches_message
is_comment_too_short
scan_mod_mail

Impure:
write_saved_id(uses logging and file I/O)
read_saved_id(uses logging and file I/O)
init(logging, praw calls)
send_first_time_message(praw calls)
get_message(random numbers)
award_points(logging, calls other impure functions)
get_this_months_scoreboard(praw calls)
update_monthly_scoreboard(logging, praw calls)
adjust_point_flair(praw calls)
already_replied(praw calls)
is_parent_commenter_author(praw calls)
points_already_awarded_to_ancestor(praw calls)
scan_comment(logging, praw calls)
scan_comments(logging, praw calls, calls to impure functions)
command_add(praw calls)
is_moderator(praw calls)
scan_message(logging, praw calls, system calls)
rescan_comment(praw calls)
rescan_comments(praw calls, calls to impure functions)
scan_comment_reply(logging, praw calls, calls to impure functions)
scan_inbox(logging, praw calls, calls to impure functions)
update_scoreboard(logging, praw calls, time sensitive)
get_top_ten_scores(praw calls)
get_top_ten_scores_this_month(praw calls, time sensitive)
update_wiki_tracker(logging, praw calls)
update_wiki_tracker(logging, praw calls, calls to impure functions)
go(logging, praw calls, calls to impure functions)

We don't need to purify every function, but if we can move all the side effects up to go(), init() or other high-up functions, it will make everything else MUCH easier to test.

Ideally, I'd like to pass scan_comments() a list of comments and other data, and get back a list of output actions that need to be performed. In the meantime, if someone could get a unit testing library set up to test the pure functions, that'd be awesome.

[Timidger]

Another thing that could really help contributions (and keeping functions "pure") would be to split up deltabot.py into separate modules. Even just having a separate module for the functions outside of the class would really help. That monster class just seems like a mess, especially with the iffy initialization function (does the bot really need to know if we are in "testing mode?" Lets just pass in a reddit object and deal with that issue outside of talking to the server). I'm going to take a look at it later tonight probably, see if I can't find some solutions if I split the code up a bit.

[Timidger]

deltabot.py is also rather defiant of PEP8 (code standards). Here are the errors from a static checker:

test.py:4:80: E501 line too long (80 > 79 characters)
test.py:5:80: E501 line too long (80 > 79 characters)
test.py:6:80: E501 line too long (80 > 79 characters)
test.py:7:80: E501 line too long (80 > 79 characters)
test.py:8:80: E501 line too long (80 > 79 characters)
test.py:9:80: E501 line too long (80 > 79 characters)
test.py:10:80: E501 line too long (80 > 79 characters)
test.py:11:80: E501 line too long (80 > 79 characters)
test.py:12:80: E501 line too long (80 > 79 characters)
test.py:13:80: E501 line too long (80 > 79 characters)
test.py:14:80: E501 line too long (80 > 79 characters)
test.py:15:80: E501 line too long (80 > 79 characters)
test.py:16:80: E501 line too long (80 > 79 characters)
test.py:17:80: E501 line too long (80 > 79 characters)
test.py:18:80: E501 line too long (80 > 79 characters)
test.py:19:80: E501 line too long (80 > 79 characters)
test.py:20:80: E501 line too long (80 > 79 characters)
test.py:21:80: E501 line too long (80 > 79 characters)
test.py:22:80: E501 line too long (80 > 79 characters)
test.py:23:80: E501 line too long (80 > 79 characters)
test.py:24:80: E501 line too long (80 > 79 characters)
test.py:61:47: E711 comparison to None should be 'if cond is not None:'
test.py:70:21: E261 at least two spaces before inline comment
test.py:141:80: E501 line too long (85 > 79 characters)
test.py:147:9: E303 too many blank lines (2)
test.py:150:80: E501 line too long (96 > 79 characters)
test.py:161:39: E127 continuation line over-indented for visual indent
test.py:165:34: E126 continuation line over-indented for hanging indent
test.py:174:80: E501 line too long (80 > 79 characters)
test.py:177:5: E303 too many blank lines (2)
test.py:187:5: E303 too many blank lines (2)
test.py:195:5: E303 too many blank lines (2)
test.py:206:5: E303 too many blank lines (2)
test.py:225:5: E303 too many blank lines (2)
test.py:230:32: E711 comparison to None should be 'if cond is None:'
test.py:251:5: E303 too many blank lines (2)
test.py:255:5: E303 too many blank lines (2)
test.py:261:80: E501 line too long (87 > 79 characters)
test.py:276:5: E303 too many blank lines (2)
test.py:283:5: E303 too many blank lines (2)
test.py:294:17: E129 visually indented line with same indent as next logical line
test.py:302:5: E303 too many blank lines (2)
test.py:307:80: E501 line too long (80 > 79 characters)
test.py:308:21: E128 continuation line under-indented for visual indent
test.py:309:21: E128 continuation line under-indented for visual indent
test.py:311:80: E501 line too long (85 > 79 characters)
test.py:329:80: E501 line too long (86 > 79 characters)
test.py:340:21: E128 continuation line under-indented for visual indent
test.py:348:5: E303 too many blank lines (2)
test.py:353:80: E501 line too long (82 > 79 characters)
test.py:354:80: E501 line too long (90 > 79 characters)
test.py:384:80: E501 line too long (103 > 79 characters)
test.py:385:53: E128 continuation line under-indented for visual indent
test.py:389:80: E501 line too long (85 > 79 characters)
test.py:393:5: E303 too many blank lines (2)
test.py:401:5: E303 too many blank lines (2)
test.py:407:5: E303 too many blank lines (2)
test.py:416:76: E502 the backslash is redundant between brackets
test.py:441:80: E501 line too long (80 > 79 characters)
test.py:442:80: E501 line too long (80 > 79 characters)
test.py:454:80: E501 line too long (87 > 79 characters)
test.py:455:80: E501 line too long (97 > 79 characters)
test.py:458:27: E126 continuation line over-indented for hanging indent
test.py:465:80: E501 line too long (80 > 79 characters)
test.py:477:5: E303 too many blank lines (2)
test.py:491:5: E303 too many blank lines (2)
test.py:507:5: E303 too many blank lines (2)
test.py:511:5: E303 too many blank lines (2)
test.py:547:5: E303 too many blank lines (2)
test.py:557:5: E303 too many blank lines (2)
test.py:574:5: E303 too many blank lines (2)
test.py:612:80: E501 line too long (81 > 79 characters)
test.py:615:80: E501 line too long (80 > 79 characters)
test.py:640:17: E265 block comment should start with '# '
test.py:650:80: E501 line too long (111 > 79 characters)
test.py:651:15: E128 continuation line under-indented for visual indent
test.py:652:15: E128 continuation line under-indented for visual indent
test.py:653:15: E128 continuation line under-indented for visual indent
test.py:654:15: E128 continuation line under-indented for visual indent
test.py:655:15: E128 continuation line under-indented for visual indent
test.py:656:15: E128 continuation line under-indented for visual indent
test.py:658:80: E501 line too long (80 > 79 characters)
test.py:671:80: E501 line too long (99 > 79 characters)
test.py:674:80: E501 line too long (97 > 79 characters)
test.py:677:80: E501 line too long (107 > 79 characters)
test.py:678:11: E128 continuation line under-indented for visual indent
test.py:679:11: E128 continuation line under-indented for visual indent
test.py:680:11: E128 continuation line under-indented for visual indent
test.py:681:11: E128 continuation line under-indented for visual indent
test.py:696:59: E126 continuation line over-indented for hanging indent
test.py:696:80: E501 line too long (80 > 79 characters)
test.py:704:59: E126 continuation line over-indented for hanging indent
test.py:705:80: E501 line too long (80 > 79 characters)
test.py:718:5: E303 too many blank lines (2)
test.py:723:80: E501 line too long (89 > 79 characters)
test.py:738:80: E501 line too long (89 > 79 characters)
test.py:742:80: E501 line too long (83 > 79 characters)
test.py:743:80: E501 line too long (90 > 79 characters)
test.py:748:15: E111 indentation is not a multiple of four
test.py:749:15: E111 indentation is not a multiple of four

[chrisuehlinger]

Hey, I've been busy (and then on vacation) for the past two months. I don't know if I'll be able to continue contributing to this project, as I'll be pretty busy for the forsee-able future. Here are my thoughts on all of this:

deltabot.py is also rather defiant of PEP8 (code standards).

If people want to enforce PEP8 that's fine, but I considered it a low priority. A lot of the "over 80s" come from the other PEP8 infraction of using 4 space tabs, which makes keeping lines to 80 chars rather difficult. I also think PEP8 line limits and 2-space tabs result in poor readbility, but that's just my opinion. If you want to sort this out go ahead, but I think it's less important than the points below.

split up deltabot.py into separate modules.

I agree, but the more I dug into deltabot, the harder that task became. Messages in the inbox can cause the bot to recheck old comments, so the line between an "inbox scanner" module and a "comment scanner" module is a little blurry. It definitely can and should be done, but it'll require careful thought and management.

Any refactors will have a high chance of introducing new bugs, which is why I made testing the first priority. This thing gets pushed out to a pretty big subreddit, if some bug causes it to start handing out deltas left and right, it'd be difficult to correct it. So testing before deployment will be very important.

(does the bot really need to know if we are in "testing mode?" Lets just pass in a reddit object and deal with that issue outside of talking to the server)

I think you might be right here, but I can't remember for sure. The mock-Reddit I was using for testing throws away the login info, so I don't think a separate "Test account" is really needed.

A note on testing in general: When making reddit bots, pretty much everything you do is a side effect. At the moment, the best testing solution I could come up with was to mock everything. However, there are no good reddit mocks available, so I adapted one from someone else's bot. It's not a perfect way to test, but it beats setting up a private reddit instance or testing on live reddit.

[Timidger]

Hey, quick question regarding the "impure" functions: how is is_parent_commenter_author impure? All it seems to do is return some of the objects composed in the praw.objects.comment object, which doesn't seem to require a praw lookup, though I could be mistaken?

[chrisuehlinger]

It was impure before I changed it to have the lookup of parent occur in scan_comment_wrapper( ). Some of the list is out of date.—
Sent from Mailbox for iPhone

On Sun, Mar 30, 2014 at 2:06 AM, Preston Carpenter
[email protected] wrote:

Hey, quick question regarding the "impure" functions: how is is_parent_commenter_author impure? All it seems to do is return some of the objects composed in the praw.objects.comment object, which doesn't seem to require a praw lookup, though I could be mistaken?

Reply to this email directly or view it on GitHub:
#42 (comment)

[PixelOrange]

Per #58, this issue should be closed. If you are still using this issue, please finish or migrate any remaining work and then close.