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

Adds conversion comment refresh requests via Private Messages #85

Closed shrink closed 6 years ago

shrink commented 6 years ago

This is my first attempt at implementing refresh requests via Private Messages sent to the bot. I am not very familiar with nodejs so I hit a stumbling block with writing tests, — any guidance on that would be greatly appreciated. Specifically, the function privateMessageFunction is being called at the start of each Private Message test but I can’t find what that function does — where does f come from?

There are quite a few erroneous tab/space characters in the bot.js and network.js files which my editor (Atom) automatically removed. I couldn't find a way to stop Atom removing them, so my commit is polluted with those changes. Sorry :-(

Left to do before this is mergeable:

Some notes on my implementation:

When a comment has been edited and no longer contains any values that need to be converted, we could either delete the bot reply or we could edit it. I did not implement deleting the reply because I think it might be fun if instead the bot reply is edited with personality, something referencing the fact that the bot was made to do work unnecessarily. This would be a fairly easy feature to implement and would be high impact so I think it would be good to leave it as something for a first timer to implement instead of implementing it as part of this change.

Closes #67

chazzlabs commented 6 years ago

Hey @citricsquid!

Specifically, the function privateMessageFunction is being called at the start of each Private Message test but I can’t find what that function does — where does f come from?

Take a look here. On that line we're creating a new function and assigning it to helperStub.setIntervalSafely. The first argument of that function is f, and we're assigning that value to either commentFunction or privateMessageFunction. helperStub is a stubbed version of the helper module which helps facilitate unit testing. You can see in the helper module that the setIntervalSafely function is calling Javascript's native setInterval function. This function calls its first argument, a callback function, every 'x' milliseconds, where 'x' is the second argument passed into the function (in this case, seconds * 1000).

Stubbing out this function in our unit test means that the setInerval function is not actually called; instead our stub is called. This is useful in this case because we don't want our unit tests executing code that may 1) delay the execution of the subsequent test cases/suites or 2) cause the unit test's assertions to be run before the callback in the setInterval function, thus causing tests to fail. Because our stub is called, this allows us to save the f function received via the first argument and call it explicitly to facilitate the execution of our test cases.

Ultimately, the answer to your question, "Where does f come from?" is: lines 31 and 32 of the helper module.

I apologize if this answer is much more detailed than you might need, but I wanted to be thorough in case anyone else reading this might benefit from it. Please let me know if you have questions.

shrink commented 6 years ago

@chazzlabs that's a perfect level of detail :) thank you! I'll work on adding some tests now.

chazzlabs commented 6 years ago

@citricsquid Awesome! I'm glad that was helpful. If you have any other questions please don't hesitate to post them here.

shrink commented 6 years ago

I have refactored the network code for getting comments and their replies. I have added 3 tests for the refresh request functionality. I'm not super confident in the quality of the tests but I think they're in the right direction. Any pointers for improvements would be great! Thank you.

shrink commented 6 years ago

I have added the refresh conversion link to the footer of the comments and refactored the process of compiling the comment footer. Each item in the footer is now managed through an array of footer items, and there is a function for generating reddit compose message links which makes use of the stopMessage value.

cannawen commented 6 years ago

I think it would be hilarious if the bot sassed people who edited their comments. @citricsquid can you open a separate issue for that feature request? XD

shrink commented 6 years ago

Thank you very much :-) I had fun! This is a great project, I like how well managed it is. Very much in the open source spirit.

I have added a feature request issue for the bot to sass users, #90. If I find time tomorrow I will go through and add some instructions (as you did in #79) so that a first timer can take a shot at implementing it.

I don't need to be added to the contributors.

nalinbhardwaj commented 6 years ago

Interestingly, after this commit, it appears tests on my computer are failing. There is no apparent reason(no diff from cannawen/master), but I'm getting the following error: screen shot 2017-10-11 at 7 31 03 am

Repeated 5 times across tests. Using node v8.6.0 and npm 5.4.2(both are >= the travis CI) on macOS.

shrink commented 6 years ago

@nalinbhardwaj I am also running macOS (High Sierra, version 10.13 (17A405)) — with node v8.6.0, npm v5.4.2 — and the tests pass for me:

  reply_maker
    #formatReply()
      ✓ should create a tabular response for 1 conversion
      ✓ should create tabular response for 2 conversions
      ✓ should create tabular response of supersets of text
      ✓ should say metric units bot on normal subreddits
      ✓ should say metric units human on totallynotrobots
      ✓ should include footer links

I am unsure what the problem could be, as looking at that line (33 of reply_maker.js) I can't think why item.value would not exist: maybe your update of the repository contents failed in some way?