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

Personality architecture refactor #103

Closed chazzlabs closed 6 years ago

chazzlabs commented 6 years ago

PR Checklist

cannawen commented 6 years ago

I do think there should be some documentation, perhaps in the Contribute Code section? Wherever it makes sense to you :)

cannawen commented 6 years ago

Still waiting for #105 to merged... I poked them earlier so hopefully it won't be too much longer :P

Are there more changes to this PR that you haven't pushed up yet?

chazzlabs commented 6 years ago

@cannawen Are we still also waiting on #90 before merging this? You mentioned waiting on that issue and #97 in a previous comment.

I do still need to address the changes you brought up, but they should be quick and easy to knock out.

cannawen commented 6 years ago

Sorry for the confusion, #90 is the issue, and #105 is the PR.

It's merged in now, so yay!

chazzlabs commented 6 years ago

@cannawen Just wanted to give you a heads up that this will be complete in the next day. Sorry for the delay on this.

cannawen commented 6 years ago

No worries! :)

chazzlabs commented 6 years ago

@cannawen I was finally able to address the changes for this PR. I apologize again for the delay.

I'm not sure what the conflicts are. I thought I'd addressed them for package.json and src/bot.js. src/personality.js no longer exists in my branch, so I'm not sure about resolving that conflict.

Taking a look at the files in my branch, it looks like there should be no issues, right?

https://github.com/chazzlabs/metric_units_reddit_bot/blob/personality-refactor/package.json#L12 https://github.com/chazzlabs/metric_units_reddit_bot/blob/personality-refactor/src/bot.js#L30

One thing I didn't change, however, is the case (in)sensitivity for personality matches. I'm not sure if we want to handle that as a change for this PR or as a separate issue.

cannawen commented 6 years ago

I fixed the merge conflicts (since the branch was behind master) :) Thanks!

The bot is slightly shadowbanned right now, I asked the admins why and hopefully it'll get unbanned. But we probably won't do any work while it's banned