Ocelots -- Eva Liu #8
Conversation
| @@ -0,0 +1 @@ | |||
| def test_moves_movie_from_watchlist_to_watched(): | |||
There was a problem hiding this comment.
Committing a new file is no issue for this project, but going forward, we generally don't want to commit temporary changes for testing, or changes to files that we don't intend to purposefully share with other folks. This can mean having to be more selective when staging files for commit.
| # Assert | ||
| assert len(updated_data["watchlist"]) == 0 | ||
| #Assert | ||
| assert len(updated_data["watchlist"]) == 1 |
There was a problem hiding this comment.
I like the additions here that test a few more scenarios! I want to encourage you to continue thinking about scenarios and further test cases, but to make it easier for folks providing feedback, in future projects I would recommend adding a new test with the data and assertions you want to check over modifying code in existing tests. We don't generally want folks changing the existing data and assertions in test files since it can take a bit of time to investigate the changes in each test and ensure they still cover what we expect.
| assert updated_data["watched"][0]["title"] == MOVIE_TITLE_1 | ||
| assert updated_data["watchlist"][0] == FANTASY_1 |
There was a problem hiding this comment.
Nice assertions for the titles 😄 In cases like these where we're checking individual keys of a dictionary, I recommend including assertions for all of the relevant keys.
watched_movie = updated_data["watched"][0]
assert watched_movie["title”] == MOVIE_TITLE_1
assert watched_movie["rating"] == RATING_1
assert watched_movie["genre"] == GENRE_1
...| assert INTRIGUE_3 in friends_unique_movies | ||
| assert HORROR_1 in friends_unique_movies | ||
| assert FANTASY_4 in friends_unique_movies |
There was a problem hiding this comment.
Great checks for all the relevant movies!
| assert len(recommendations) == 0 | ||
| assert recommendations == [] |
There was a problem hiding this comment.
The first assertion will guarantee that recommendations is a sequence or collection that is empty, while the second assertion will only pass if recommendations is an empty list. Since they give us similar information, we could remove one of these lines.
| non_watched_list.append(movie) | ||
| # print(f"non_watched_list: {non_watched_list}") | ||
| return non_watched_list | ||
| #wave 3 fun 2 |
There was a problem hiding this comment.
In these lower waves we've lost blank lines that give us a visual separation between functions. The PEP8 style guide doesn't say much about blank lines between functions when they aren't part of a class, but I would suggest following the guideline for functions in a class by using a single blank line to give readers a visual separation between functions.
Even if there is a comment between functions, when we have a long wall of text, a comment is not as visually distinct as a blank line, so it generally takes longer for our brains to register. When you're reading through a large code base to familiarize yourself with it, every little thing that makes code easier to read and understand is appreciated.
https://peps.python.org/pep-0008/#blank-lines
| # print(f"non_watched_list: {non_watched_list}") | ||
| return non_watched_list | ||
| #wave 3 fun 2 | ||
| def get_friends_unique_watched(user_data): |
There was a problem hiding this comment.
The feedback in get_unique_watched applies here as well.
| user_subscriptions_list = user_data["subscriptions"] | ||
| recommended_movies_list = [] | ||
| for movie in user_nonwatched_list: | ||
| for host_name in user_subscriptions_list: | ||
| if movie["host"] == host_name: | ||
| recommended_movies_list.append(movie) |
There was a problem hiding this comment.
Nice algorithm! Another way we could approach filtering the unique_watched list is with a list comprehension:
# list comprehension
result = [movie for movie in user_nonwatched_list if movie["host"] in user_data["subscriptions"]]This line is a bit long, in practice we would split a statement like this across lines, use some extra variables, or shorten some naming to keep under the PEP8 guide of 79 characters max per line:
# list comprehension
result = [movie for movie in user_nonwatched_list
if movie["host"] in user_data["subscriptions"]]| # user didn't watch but friend watched | ||
| # match the user's subscriptions list | ||
| # may use get_friends_unique_watched(user_data) to return a user non_watched list | ||
| user_nonwatched_list = get_friends_unique_watched(user_data) |
| recommended_movies_list.append(movie) | ||
| return recommended_movies_list | ||
|
|
||
| def get_rec_from_favorites(user_data): |
There was a problem hiding this comment.
This implementation gets us what we need, but I think we could benefit by asking questions to see what we could simplify. Some to start with could be:
- There are several functions in this file that take a list of movies create a set of just the titles. What helper functions could we create to reduce duplication of the set creation code?
- Are there functions we already wrote that could be used to help solve this problem?
|
Hi Eva, thanks for the questions! As a heads up, I'll usually respond to
feedback questions on GitHub, but for some reason I don't see these
comments on the Pull request, so I've copied the questions below to reply
here.
*Hi Kelsey, I have a question. If after I did the pull request, I want to
change my commit or files from my repository but don't intend to share it
with other folks. Should I close my pull request and redo the pull request?
Thank you!*
*- *If you want to make changes but don't want to push them to the same
branch the pull request was made against, you could create a new branch
from master. It would contain everything on master so far, but new changes
wouldn't affect the original branch.
*When you said adding a new test, would it mean it is better I create a new
test file ex: test_wave_01_addition.py and write my test in there, or do I
need to create a new test with (test_function/# Arrange/# Assert) in the
test_wave_01.py? Thank you!*
- I would probably add a new test function to the existing file rather than
creating a new file, but it is up to you, either way is valid and leaves
the existing tests as they started.
*I do have an issue with this project when I am trying to use the debugger.
I couldn't open the debugger when I am running this project*.
- We'll make sure to walk through examples that use the debugger in the
first few class sessions, but if you're still having issues with running
the debugger in VS Code, I would recommend dropping by office hours or
posting in #study-hall on Slack with what you're seeing so folks can help
you troubleshoot and get up and running with the debugger.
Please reach out if you have further questions or if there's anything I can
clarify =]
Kelsey Steven
…On Fri, Nov 11, 2022 at 2:38 PM Eva Liu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In viewing_party/party.py
<#8 (comment)>:
> + print(f"final user_data: {user_data}")
+ print(len(user_data["watchlist"]))
I do have an issue with this project when I am trying to use the debugger.
I couldn't open the debugger when I am running this project.
—
Reply to this email directly, view it on GitHub
<#8 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXFAFB75UJ3EYV5WEOK6CRTWH3DHXANCNFSM6AAAAAAR25VJCE>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
Kelsey Steven
*Associate Software Development Instructor*
pronouns.they/them
315 5th Ave South Suite 200 | Seattle, Washington 98104
Telephone: 224.225.5232
***@***.*** | adadevelopersacademy.org
FOLLOW US on Facebook <https://www.facebook.com/adadevelopers/>, Twitter
<https://twitter.com/adaacademy?lang=en>, YouTube
<https://www.youtube.com/channel/UCA8egkZmcVHktjW5oRIT-DA>, and LinkedIn
<https://www.linkedin.com/company/ada-developers-academy>
|
Hi Kelsey, Best, |
No description provided.