-
Notifications
You must be signed in to change notification settings - Fork 29
Sphinx - Sarah Koch and Salma Anany #22
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
…dditional assertion statements are still skipped.
…m_watchlist_to_empty_watched() in test_wave_01.py, adds assert statements to test that the correct movie was added to "watched"
…atched(). Adds assertions to test that the correct movie was added to "watched". Removes raised exception and comments out skip decorator.
…_movie_from_watchlist_to_watched(), which checked against GENRE_1 and RATING_1 instead of movie_to_watch["genre"] and movie_to_watch["rating"], which could cause tests to falsely report as failed.
…mented out in test_wave_02.py. Wave 2 user data uncommented out in play_tester.py.
…r waves. Comments out all skip decorators in test_wave_05.py, completes test code for test_new_genre_rec_from_empty_friends(). Adds new function get_new_rec_by_genre() to Wave 5 in party.py, passing first 3 wave 5 tests. Cleans up inconsistent whitespace between functions in party.py.
… 5 tests (returns empty list if user has no favorites, returns user favorites if friends have watched no movies).
…s. Refactoring, playtesting not yet complete.
Co-authored-by: SalmaAnany <[email protected]>
Co-authored-by: SalmaAnany <[email protected]>
…n to bottom of wave for readability.
…s all existing docstrings use identical display formatting.
…hitespace in Wave 4.
…tes(). Changes some variable names for clarity.
… Wave 3. Makes docstrings more concise and to the point. Makes additional changes for clarity and standardization, including using comprehensions where appropriate, formatted for readability.
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 viewing-party, Sarah and Salma!
I also see that you two made frequent commits, nice job!
Let me know if you have any questions about my review comments.
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.
Did you mean to add and commit these files in the .idea directory? I don't think these changes are needed for the project and should be removed from the tracked changes before opening a PR.
While adding these changes as a PR, doesn't affect this project right now, when you're in industry it's best practice to only commit changes relevant to your work so that the codebase doesn't become cluttered. Be sure to review what files you have staged for commit before you add them so that you don't add files that shouldn't be tracked to the commit history.
assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | ||
assert updated_data["watched"][0]["genre"] == GENRE_1 | ||
assert updated_data["watched"][0]["rating"] == RATING_1 |
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 job adding assertions to this test 👍
assert updated_data["watched"][1]["title"] == movie_to_watch["title"] | ||
assert updated_data["watched"][1]["genre"] == movie_to_watch["genre"] | ||
assert updated_data["watched"][1]["rating"] == movie_to_watch["rating"] |
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 could capture the logic written in lines 173-175 as:
assert HORROR_1 in updated_data["watched"]
Also since you're testing that HORROR_1 is in the watched list, you might also to test that HORROR_1 is not in the watchlist.
assert INTRIGUE_3 in friends_unique_movies | ||
assert FANTASY_4 in friends_unique_movies | ||
assert HORROR_1 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.
👍 Nice job checking that the correct movies are in friends_unique_watched
. Consider adding an assertion to check that the movie that was added to the first friend's watched list on line 50 was not added to Amanda's watched list:
assert INTRIGUE_3 not in amandas_data["watched"]
|
||
@pytest.mark.skip() | ||
# 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.
Writing the assertion len(recommendations) == 0
only enforces that recommendations is something we can get the length of (and it's empty). If we write the assertion like assert recommendations == []
it guarantees the thing in recommendations is actually a list (and it's empty).
return ([rec for rec in available_recs | ||
if rec['genre'] == most_watched_genre | ||
and not any(movie['title'] == rec['title'] | ||
for movie in user_data['watched'])]) |
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 is also pretty dense and a little difficult to read. With a list comprehension like this, I might suggest writing it out in for-loops so you don't compromise readability.
for friend in user_data['friends'])): | ||
return [] | ||
|
||
available_recs = 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.
Why do we call get_available_recs
? This function returns a list of movies where a user hasn't watched, but a friend has, and the "host" of the movie is a service that is in the user's "subscriptions"
For this function, we want to recommend movies that a user hasn't watched, but friends have watched, and the movie's genre is the same as the most_watched_genre
. So we don't need to consider a movie's host when adding a movie to the list of recommended movies here.
We could use get_friends_unique_watched(user_data)
though to get all the movies only watched by friends and then compare the genres of those movies to most_watched_genre
friends_watched_titles = ({ | ||
movie['title'] for friend in user_data['friends'] | ||
for movie in friend['watched'] | ||
}) |
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.
Could we use get_friends_watched_list()
here instead?
rec_from_favorites = ([ | ||
favorite for favorite in user_data['favorites'] | ||
if favorite['title'] not in friends_watched_titles | ||
]) |
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 is starting to stretch how much we should put into comprehension. For me, this isn't as readable as it could be if we used a for loop.
What about something like
def get_rec_from_favorites(user_data):
friends_movies = get_friends_watched_list(user_data)
fav_movie_rec_list = []
for movie in user_data["favorites"]:
if movie not in friends_movies:
fav_movie_rec_list.append(movie)
return fav_movie_rec_list
genre_count = {} | ||
for movie in user_data['watched']: | ||
genre = movie['genre'] | ||
genre_count[genre] = genre_count.get(genre, 0) + 1 | ||
|
||
most_watched_genre = None | ||
max_count = 0 | ||
for genre, count in genre_count.items(): | ||
if count > max_count: | ||
max_count = count | ||
most_watched_genre = genre | ||
|
||
return most_watched_genre |
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.
👍
No description provided.