search

cannawen / metric_units_reddit_bot

Reddit bot converting imperial units to metric units
https://www.reddit.com/user/metric_units
GNU General Public License v3.0
78 stars 34 forks source link

Add personality response for 'I would walk 500 miles' #96

Closed sirugh closed 6 years ago

sirugh commented 6 years ago

PR Checklist

cannawen commented 6 years ago

Hey @sirugh ! Thank you for the PR :) This one is tricky, there is actually more to do than just adding the response to personality.js

The responses in personality.js are triggered upon reading a private message, but the "I would walk 500 miles" is more like a conversion, in which it's triggered by searching through all new comments. It should be programmed more like the "totallynotrobots" special case in bot.js.

I'm going to close this PR. But I can assign #65 to you, if you be interested in adding the special case to bot.js? We would be eternally grateful :D

sirugh commented 6 years ago

Sure I can do that, thanks for the pointer!

sirugh commented 6 years ago

@cannawen after looking into it, the case for totallynotrobots is a bit confusing. In fact, if you look here it looks like the bot actually determines the personality response based on the incoming comment (to match a regex). The totallynotrobots case occurs immediately below it, but it only checks for messages in that specific subreddit.

What I'm trying to say is that my changes from this PR should have worked as intended. An incoming message to the bot would have passed through the logic I linked, thus creating the intended response.

cannawen commented 6 years ago

@sirugh You're correct, it's not exactly like totallynotrobots, sorry for being unclear!

We want to search new comments for "I would walk 500 miles" which is going to come from the network call network.getRedditComments("all") (in postConversions function), and not network.getUnreadMessages();

I think it would make sense to rename postConversions to handleNewComments (or similar) and then add the 500-mile code in there, before we check for what conversions exist. What do you think?

cannawen commented 6 years ago

Also FYI, a PR was just merged into master that makes some heavy bot.js changes, so it would make sense to pull down the latest code