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

Scissors - Mariela C #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

marielaxcruz
Copy link

No description provided.

Copy link
Contributor

@beccaelenzil beccaelenzil left a comment

Choose a reason for hiding this comment

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

Good work creatively implementing the Video Store CLI. You are off to a good start, and the work your put into the user interface makes for a great user experience. You have successfully handled many of the user stories. I have left several in-line comments focused on places where your CLI code should handle 400 level responses. Consider how you could implement these behaviors. Please let me know what questions you have.

@@ -0,0 +1,6 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

For this project we needed to add a .gitignore

Comment on lines +167 to +168
for customer in customer.list_customers():
print(customer)
Copy link
Contributor

Choose a reason for hiding this comment

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

In this loop we accidently override the customer instance with a customer dictionary that is returned in the json from list_customers. This raises an exception later on when we try to run the line customer.print_selected. Consider naming your customer instance something like customer_manager since it provides all the methods to handle the customer routes.

json=query_params
)

print("response:", response)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method returned a 400 response {'message': 'Invalid data'}. Consider what data is required for the request.

print(customer.selected_customer)

checked_in_rental = rental.check_in(customer_id, video_id)
print(checked_in_rental)
Copy link
Contributor

Choose a reason for hiding this comment

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

Attempting the check in a video that did not exist as a rental resulted in this response <Response [404]> Success ! The video was checked in!. Consider how your CLI could handle the 404. Your API does the hard work of determining that rental doesn't exist, not it's your CLI's job to let the user know.

Comment on lines +202 to +204
checked_out_rental = rental.check_out(customer_id, video_id)
print("Success! The video was checked out!")
print(checked_out_rental)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as with the check-in, consider how your CLI should handle the situation where the check-out cannot be completed (for instance, due to not available inventory).

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.

2 participants