Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cannawen opened this issue Oct 8, 2017 · 27 comments · Fixed by #80
Closed

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

cannawen opened this issue Oct 8, 2017 · 27 comments · Fixed by #80

Comments

@cannawen
Copy link
Owner

cannawen commented Oct 8, 2017

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

  • Programming fundamentals (if statements, arrays, etc.)
  • No previous experience working with Regular Expressions
  • Some familiarity with how Reddit works

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
Copy link

I'd Like to work on this Issue.

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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
Copy link
Contributor

namantw commented Oct 8, 2017

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

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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
Copy link
Owner Author

cannawen commented Oct 8, 2017

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
Copy link
Contributor

namantw commented Oct 8, 2017

Okay, just found it. Thanks!

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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
Copy link
Contributor

namantw commented Oct 8, 2017

Okay, thanks a lot for your help :)

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

It passed all the tests (Hurray!)
image

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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
Copy link
Contributor

namantw commented Oct 8, 2017

Okay done!

namantw added a commit to namantw/metric_units_reddit_bot that referenced this issue Oct 8, 2017
Fixed issue : "oz" is not being converted to "troy oz" in subreddit /r/Pmsforsale cannawen#79. Added a 'gi' flag in the specialSubredditsRegex in ./src/conversion_helper.js
namantw added a commit to namantw/metric_units_reddit_bot that referenced this issue Oct 8, 2017
@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

@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
Copy link
Owner Author

cannawen commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

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

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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

@namantw
Copy link
Contributor

namantw commented Oct 8, 2017

It shows this
image

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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
Copy link
Contributor

namantw commented Oct 8, 2017

Thanks for your patience and support :)

@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

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 cannawen mentioned this issue Oct 8, 2017
3 tasks
@cannawen
Copy link
Owner Author

cannawen commented Oct 8, 2017

^ @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:

  • Missing environment.yaml instruction
  • all-contributors thing did not work.
  • Unclear instructions on how the PR should be formatted

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

@namantw
Copy link
Contributor

namantw commented Oct 9, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@cannawen @arpitbatra123 @namantw and others