-
Notifications
You must be signed in to change notification settings - Fork 29
Phoenix C22- Tatyana Venanzi #12
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on your first pair project Kristina & Tatyana!
.gitignore
Outdated
@@ -143,3 +143,4 @@ dmypy.json | |||
|
|||
# Cython debug symbols | |||
cython_debug/ | |||
tests/test_wave_01.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this an intentional change to the .gitignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not intentional at all, is that something we would need to fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can go back and fix it if you'd like the practice, but it's mostly something to really keep an eye out for in the future. .gitignore
should only contain files that we want to keep out of version control (out of the git repo).
tests/test_wave_01.py
Outdated
# ******************************************************************************************* | ||
# ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
# ******************************************************************************************* | ||
# assert MOVIE_TITLE_1 not in updated_data["watchlist"] | ||
# assert MOVIE_TITLE_1 in updated_data["watched"] | ||
assert janes_data not in updated_data["watchlist"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like y'all to revisit this test if you have time. I don't believe this assert is doing what you intend. janes_data
is the full structure:
janes_data = {
"watchlist": [{
"title": MOVIE_TITLE_1,
"genre": GENRE_1,
"rating": RATING_1
}],
"watched": []
}
watched
is a part of this data structure, so we likely would not want to try referencing the structure holding it inside of watched
. We also cannot add mutable data structures like janes_data
as keys to a dictionary, so janes_data not in updated_data["watchlist"]
is always going to evaluate to True.
We want to confirm that the items in the watched
and watchlist
changed how we expected.
- What syntax do we need to access the data inside of the
watched
list? - What values inside of that list do we want to confirm to ensure that the movie keys are each what we expect?
# ******************************************************************************************* | ||
# ****** Add assertions here to test that the correct movie was added to "watched" ********** | ||
# ******************************************************************************************* | ||
assert movie_to_watch not in updated_data["watchlist"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of in
to check all the dictionary values at once!
We assert that movie_to_watch
is not in updated_data["watchlist"]
, but does that alone confirm that is has been moved to watched
?
What other asserts to we need to add to be certain that each of the movies with the correct data in the lists where they belong, and no data was altered when moving them from list to list?
# ************************************************************************************************* | ||
# ****** Add assertions here to test that the correct movies are in friends_unique_movies ********** | ||
# ************************************************************************************************** | ||
assert INTRIGUE_3 in friends_unique_movies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
friends_unique_movies
contains 3 values, so we should write assert statements to confirm ALL of the contents are what we expect. What other movies should be in the list, and how can we test for them??
tests/test_wave_05.py
Outdated
# raise Exception("Test needs to be completed.") | ||
# Act | ||
recommendations = get_new_rec_by_genre(sonyas_data) | ||
# ********************************************************************* | ||
# ****** Complete the Act and Assert Portions of these tests ********** | ||
# ********************************************************************* | ||
#Assert | ||
assert len(recommendations) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Act & Assert!
To make it easier to understand what's happening at a glance, I recommend removing the pre-existing exception and comment, since they aren't necessary once the test is completed. Right now the act and assert sections of the test blend into one code chunk. With the comments removed, we should use new lines to create visual space and separate the steps.
viewing_party/party.py
Outdated
|
||
#loops through movies in the total_friends_watched list to compare (in the if statement) if the movie is not in the user's watched list and hasn't appeared in the unique_movies_list (checks for duplicates) to then append the unique movie | ||
for unique_movies in total_friends_watched: | ||
if (unique_movies not in user_data["watched"]) and (not unique_movies in unique_movies_list): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is a bit long, I’d suggest breaking this statement apart. The PEP 8 style guide recommends limiting lines to a max of 79 characters to make code easier to read, especially with multiple editor windows open: https://peps.python.org/pep-0008/#maximum-line-length.
# ----------------------------------------- | ||
# ------------- WAVE 4 -------------------- | ||
# ----------------------------------------- | ||
|
||
def get_available_recs(user_data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid approach! Since we need to consider only movies that one of our friends have watched but we have not, how could we reuse our previous functions get_friends_unique_watched
to simplify the code?
viewing_party/party.py
Outdated
# loop through their moviees | ||
for friend_movie in friend["watched"]: | ||
# check if user hasn't watched the movie and if host (streaming services) is also under user | ||
if (friend_movie["title"] not in user_watched_list and friend_movie["host"] in user_data["subscriptions"] and friend_movie not in recommendations): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be split up for length & readability
# ----------------------------------------- | ||
# ------------- WAVE 5 -------------------- | ||
# ----------------------------------------- | ||
|
||
def get_new_rec_by_genre(user_data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_new_rec_by_genre
is duplicating work that we've already done to find the most common genre for a user and only look at movies that at least one friend has seen, but we have not. How can we reuse our previous functions get_most_watched_genre
and get_friends_unique_watched
to simplify our approach here?
#returns list of recommended movies | ||
return recommended_list | ||
|
||
def get_rec_from_favorites(user_data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could we reuse our prior function get_unique_watched
to simplify the code here?
Co-worked with Kristina Nguyen