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

Sapphire - Monica Lagdaan #71

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

Sapphire - Monica Lagdaan #71

wants to merge 5 commits into from

Conversation

monicadjl
Copy link

No description provided.

Copy link

@tildeee tildeee left a comment

Choose a reason for hiding this comment

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

Hi Monica! I know that we've worked on 4+ months of content since this project was originally assigned, so a lot of context has changed.

Anyway, great work on this project! Your code looks great. In particular, your get_new_rec_by_genre and get_rec_from_favorites implementations for the final Wave 5 are really great. I really like what your code style has developed into, because your logic is clear and efficient.

Well done! LGTM!!!

# Assert
assert len(updated_data["watchlist"]) == 1
assert len(updated_data["watched"]) == 2
assert HORROR_1 == updated_data["watched"][1]["title"]
Copy link

Choose a reason for hiding this comment

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

Suggested change
assert HORROR_1 == updated_data["watched"][1]["title"]
assert HORROR_1["title"] == updated_data["watched"][1]["title"]

After looking at the definition of HORROR_1 in tests/test_constants.py, I realized that this change passes the tests!

Comment on lines +21 to +22
watchlist = user_data.get("watchlist", [])
watched = user_data.get("watched", [])
Copy link

Choose a reason for hiding this comment

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

I like this pattern, I would keep doing this when it makes sense!

if movie["title"] == title:
found_movie = movie
watchlist.remove(movie)
watched.append(movie)
Copy link

Choose a reason for hiding this comment

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

Nitpick: I like how this reads since watched = user_data.get("watched", []) etc, but this is also a good opportunity to use the previous functions:

Suggested change
watched.append(movie)
add_to_watched(user_data, movie)

Comment on lines +86 to +95

user_watched = set([movie["title"] for movie in user_data["watched"]])
friends_list = []

for friend in user_data["friends"]:
for movie in friend["watched"]:
if movie["title"] not in user_watched and movie not in friends_list:
friends_list.append(movie)

return friends_list
Copy link

Choose a reason for hiding this comment

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

This is probably one of the best implementations/directions to go with this function, your code looks clean and robust, well done!

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