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

FOOD-23 Input $$ as an argument for price range #24

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

josephthweatt
Copy link
Contributor

@josephthweatt josephthweatt commented Oct 22, 2019

What does this PR do?

Gimme eat with $money$

Who is reviewing it (please choose AT LEAST two reviewers that need to approve the PR before it can get merged)?

@aaronilovici
@aperullo
@cjlange
@josephthweatt
@mdang8
@oconnormi
@pvargas
@rfding
@njbarber

How should this be tested? (List steps with links to updated documentation)

Enter in five dollar signs as an argument. Ensure that a fancy restaurant is recommended. Go to restaurant to confirm assessed swankiness.

Any background context you want to provide?

$$$$$$$$$

What are the relevant tickets?

Fixes #23

Screenshots (if appropriate)

image

Checklist:

  • Documentation Updated
  • Update / Add Unit Tests
  • Update / Add Integration Tests

@josephthweatt josephthweatt changed the title Food 23 FOOD-23 Input $$ as an argument for price range Oct 22, 2019
Copy link
Contributor

@mdang8 mdang8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CircleCI says there are failing tests

@mdang8
Copy link
Contributor

mdang8 commented Oct 22, 2019

Please assign @njbarber as a reviewer as well.

@pvargas pvargas requested a review from njbarber October 22, 2019 19:37
@mdang8
Copy link
Contributor

mdang8 commented Oct 23, 2019

CircleCI still failing. You can run pytest -v --cov-report term --cov=src tests/ locally to see the failing tests

Comment on lines -57 to +60
is_valid = False
is_invalid = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y tho

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra argument means there's logic changes in how we look at the command. Its simpler to say that the command is valid and must be proven otherwise, than to assume it's wrong and must be proven valid.

@mdang8
Copy link
Contributor

mdang8 commented Oct 24, 2019

Can we change it so users can instead pass in a number representing the highest dollar amount they are willing to spend rather than $ symbols? I'm matt dang and I think joseph's pull request is perfect

@josephthweatt
Copy link
Contributor Author

josephthweatt commented Oct 24, 2019

@mdang8 we could do that, but then we would have two numerical arguments and no real way to convey the order of the arguments. Also, we couldn't set the price range unless we set the choices first. I do have the API work with actual numbers in the parameters, though.

Also, I don't think $ is more fun!

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

Successfully merging this pull request may close these issues.

Input $$ as an argument for price range
2 participants