Skip to content

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

Open
josephthweatt wants to merge 8 commits into
masterfrom
FOOD-23
Open

FOOD-23 Input $$ as an argument for price range#24
josephthweatt wants to merge 8 commits into
masterfrom
FOOD-23

Conversation

@josephthweatt

@josephthweatt josephthweatt commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

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

@mdang8 mdang8 left a comment

Copy link
Copy Markdown
Contributor

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

mdang8 commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

Please assign @njbarber as a reviewer as well.

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

mdang8 commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

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

Comment thread src/lib/SlackClient.py
Comment on lines -57 to +60
is_valid = False
is_invalid = False

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

mdang8 commented Oct 24, 2019

Copy link
Copy Markdown
Contributor

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

josephthweatt commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

@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