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

"oz" is not being converted to "troy oz" in subreddit /r/Pmsforsale #79

Closed cannawen closed 6 years ago

cannawen commented 6 years ago

metric_units is a sassy reddit bot that finds imperial units, and replies with a metric conversion.

First timers only

This issue is reserved for anyone who has never made a pull request to Open Source Software. If you are not a first timer, we would still love your help! Please see our other open issues :)

Read our New to OSS guide for an overview of what to expect

IMPORTANT: Comment below if you would like to volunteer to fix this bug.


Recommended experience

Time estimate

30-60 minutes

Background Information

So, you want to work on a Reddit bot that converts imperial units to metric units? Awesome! It's not an easy problem to solve though :( Imperial units are confusing!!

Take ounces, for example. When someone says "ounces" they usually mean regular ("avoirdupois") ounces (which is 28.3495 grams). But, they could also be referring to "troy" ounces (31.1035 grams). Troy ounces are most often used when dealing with precious metals, like gold or silver

The problem

The subreddit /r/Pmsforsale is all about precious metals. They may refer to something as "ounce", but what they really mean is "troy ounce"

So, when the bot finds itself in /r/Pmsforsale, we want it to find all mentions of "ounces" and replace them with "troy ounces". This should already be happening, but it is not! There is a bug in the code.

To replace "ounces" with "troy ounces", we must find them by using a thing called Regular Expressions (also known as a, "regex"). Regexes help us find strings that match a certain pattern. For example if we had a regex a and applied it to this string: ababcA ... it would find all of the lower case a's but ignore the other characters (b, c, and A).
OPTIONAL: You can go through this tutorial to learn more about regexes

OK, we are ready to get started. Lets get our development environment set up!

If you run into any problems, try googling for a solution. If that doesn't work, reply to this issue with screenshots of the error and what steps you have already taken to try to solve the problem. We're happy to help :)

  1. Open up a terminal window and type node --version
  2. If it complains you do not have node, download it here.
  3. If your Node version is under v6.2.1, you have to update it
  4. Fork the main github repository and "clone your fork" (download the bot's code) using git. See more detailed instructions here
  5. Run npm install to download all of the required dependencies
  6. Run npm test to run all of the tests. All green? Yay!
  7. Open ./src/conversion_helper-test.js in your favourite text editor
  8. Search for the text "should convert oz to troy oz in precious metal sub"
  9. Replace it.skip with it.only. This will tell the program to only run this single test. While you're here, take a look at the test! Read it carefully, can you guess what it does?
  10. Run npm test again
  11. Observe failing test :( Boo, failing tests! Booo! Tests allow us to define inputs and expected outputs to functions, so we can see if a function is doing the correct thing.
  12. This test is failing because of a bug in the code. Let's go find that code
  13. Go to ./src/conversion_helper.js and find where we declare specialSubredditsRegex
  14. This line creates a regex, but all regexes are case-sensitive by default! Notice the difference in capitalization in our test vs. our regular expression?
  15. Your task is to make the regex match case-insensitively to make the test pass. Please use Google or any other resource.
    OPTIONAL: Can you think of other ways to make the test pass? Post your ideas in your Pull Request description later on (step 18).
  16. Once your single test is passing, change the it.only to it to run all of the tests again.
  17. Is everything green? Woohoo, green! Yay!!
  18. Please commit the changes with a descriptive git commit message, and then make a pull request back to the main repository :)
    OPTIONAL: Don't forget to give yourself credit! Thank you for contributing to metric_units bot!
  19. Wait for your PR to be reviewed by a maintainer, and address any comments they may have

Step 20: YOUR CHANGE GETS MERGED AND RELEASED!! Party!!!

arpitbatra123 commented 6 years ago

I'd Like to work on this Issue.

cannawen commented 6 years ago

Hello @arpitbatra123, this issue is reserved for people who have never opened a PR to open source. It looks like you've made a PR before, which is awesome! But it means you are overqualified for this issue :( If you are so inclined, you can check out any of the other issues in this repo that are not tagged first timers only :D

Another alternative is you can go ahead and fix the issue just for learning purposes, but your PR will not be accepted back into the main repo.

namantw commented 6 years ago

Hello @cannawen I would love to work on this issue.

cannawen commented 6 years ago

Assigned to you, @namantw! Feel free to reach out if you get stuck and can't find a solution :)

namantw commented 6 years ago

Hey, in step number 5 it says "Run npm install to download all of the required dependencies". Are there any dependencies other than mocha?

cannawen commented 6 years ago

Yes, there should be more dependencies than mocha.
You can see all the project's dependencies in the package.json file :) npm install should install them all

namantw commented 6 years ago

Okay, just found it. Thanks!

namantw commented 6 years ago

I got this when I ran npm test the very first time :( screenshot from 2017-10-08 22-03-19

namantw commented 6 years ago

What am I doing wrong? I am an absolute beginner when it comes to npm.

namantw commented 6 years ago

Okay I think I have found the "issue". I added a flag 'gi' in the code. image

cannawen commented 6 years ago

Hey @namantw , sorry I didn't see your message earlier!

I'm sorry, I forgot a step in the setup (MY BAD!). You need to move the sample_environment.yaml file into a new folder called "private" and rename it to environment.yaml.

That fix looks good to me, but try to run the tests again to see if it passes :)

namantw commented 6 years ago

Okay, thanks a lot for your help :)

namantw commented 6 years ago

It passed all the tests (Hurray!) image

cannawen commented 6 years ago

Awesome!!! Make a pull request, and I will review it :)

namantw commented 6 years ago

One more thing should I keep the private/environment.yaml as it is or revert it back to the way it was?

cannawen commented 6 years ago

Oops... I should not have told you to "move" the sample_environment.yaml, I should have told you to copy and paste. I will clear up these instructions for the next time I open a "first timers only" issue ;)

Can you revert "sample_environment.yaml" back to the way it was?

namantw commented 6 years ago

Okay done!

namantw commented 6 years ago

@cannawen I have made the pull request! But I do not know how to add myself in the 'credits'. Can you do that for me if the PR is merged.

cannawen commented 6 years ago

@namantw What have you tried so far? Can you send some screenshots if you are encountering errors?

namantw commented 6 years ago

I have made the PR. The issue is fixed, but I could not add myself to the 'credits'.

cannawen commented 6 years ago

Have you tried running this line: npm run all-contributors add namantw code ? Does it return any errors?

namantw commented 6 years ago

It shows this image

cannawen commented 6 years ago

Hmm, interesting, and this is after you have already run npm install?

OK... I'm not sure what is going wrong there either, I can add you after your PR gets merged in ;)

namantw commented 6 years ago

Thanks for your patience and support :)

cannawen commented 6 years ago

No, thank you for contributing ;)

Oh, one more idea about the all-contributors thing: Can you check git log ? Is there a commit that says something like "Added @namantw as a contributor"?

cannawen commented 6 years ago

^ @namantw See, this issue was closed automatically once the PR was merged ;)

Do you have any feedback on your experience? This is the first time I have created a "first timers only" issue, so there were a few hiccups:

Anything else I can do to improve the experience for future first-timers?

namantw commented 6 years ago

My experience was awesome :D I really appreciate your support and patience. I think you had explained everything very clearly. I don't think there is anything to improve.