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

Rock - Adelaide Nalley #52

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

adiainthesky
Copy link

No description provided.

Copy link

@spitsfire spitsfire left a comment

Choose a reason for hiding this comment

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

All right, Adelaide! While there were some errors in the CLI, you made mention of them in comments, which let me know that you were aware, but under time constraints.

What I found was that when creating a new video with blank data, it would break the CLI. One thing we could do to avoid that is make sure the user can't move on until they enter valid data. We could also do that with updating customer, videos, an creating a customer, too.

Lastly, your code in main.py was packed tightly, which made it hard to read. One thing we could do in the future is separate big chunks of code into helper functions and import them from another file.

Comment on lines +76 to +77
#I wrote URL instead of TaskList. is that right?
video_store = URL(url="BACKUP_URL")

Choose a reason for hiding this comment

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

Based on how I read this program, URL is a constant variable, not a model. So we can't create an instance of URL. I think you should still be using VideoStore, not TaskList or URL

@@ -0,0 +1,121 @@
import requests
import datetime
from flask import json

Choose a reason for hiding this comment

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

since this is a front-end CLI that simply interacts with the user in the terminal, we don't need to bring in flask, which is a backend framework. the .json() method is part of the requests module we've imported

Suggested change
from flask import json

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