Phoenix - Lula San by machilula · Pull Request #13 · Ada-C22/viewing-party

machilula · 2024-09-27T01:37:01Z

No description provided.

anselrognlie

I've added comments about the implementation as submitted. Please review them, and let me know if you have any questions. There are a few things I'd like you to revisit before we move on from this project. My Slack communication has more detail about exactly what to focus on.

anselrognlie · 2024-09-28T18:06:04Z


-    raise Exception("Test needs to be completed.")
+    #raise Exception("Test needs to be completed.")
    # *******************************************************************************************
    # ****** Add assertions here to test that the correct movie was added to "watched" **********
    # *******************************************************************************************


👀 The tests with these sorts of instructions are asking you to add your own code to complete what this test is trying to validate. The raise Exception("Test needs to be completed.") statement did need to be commented out or removed (it was just there to draw your attention), but we're also looking for additional assertions. What needs to be checked to confirm that the data moved from the watchlist to the watched list was moved correctly?

anselrognlie · 2024-09-28T18:07:04Z


-    raise Exception("Test needs to be completed.")
+    # raise Exception("Test needs to be completed.")
    # *******************************************************************************************
    # ****** Add assertions here to test that the correct movie was added to "watched" **********
    # *******************************************************************************************


👀 The tests with these sorts of instructions are asking you to add your own code to complete what this test is trying to validate. The raise Exception("Test needs to be completed.") statement did need to be commented out or removed (it was just there to draw your attention), but we're also looking for additional assertions. What needs to be checked to confirm that the data moved from the watchlist to the watched list was moved correctly? How can we be sure the correct data was left in the watchlist?

anselrognlie · 2024-09-28T18:08:27Z


-    raise Exception("Test needs to be completed.")
+    # raise Exception("Test needs to be completed.")
    # *************************************************************************************************
    # ****** Add assertions here to test that the correct movies are in friends_unique_movies **********
    # **************************************************************************************************


👀 The tests with these sorts of instructions are asking you to add your own code to complete what this test is trying to validate. The raise Exception("Test needs to be completed.") statement did need to be commented out or removed (it was just there to draw your attention), but we're also looking for additional assertions. What needs to be checked to confirm that the expected 3 movies were returned?

anselrognlie · 2024-09-28T18:10:22Z


-    raise Exception("Test needs to be completed.")
+    #raise Exception("Test needs to be completed.")
    # *********************************************************************
    # ****** Complete the Act and Assert Portions of these tests **********
    # *********************************************************************


👀 The tests with these sorts of instructions are asking you to add your own code to complete what this test is trying to validate. The raise Exception("Test needs to be completed.") statement did need to be commented out or removed (it was just there to draw your attention), but we're also looking for additional assertions. This test only provides the arrange part, and the name of the test tells us what it should be checking. What action do we need to generate a genre-based recommendation? And what do we expect the result to be when the friends haven't watched anything?

anselrognlie · 2024-09-28T18:12:59Z

@@ -1,23 +1,155 @@
 # ------------- WAVE 1 --------------------
+from tests.test_constants import *


👀 The "real" code of our project should not depend on code/constants from the tests. Imagine in a real scenario, we don't ship the tests to customers (for example, we don't have the tests that Google uses for Chrome on our system. We only have the main code. As a result, test code can (in fact, must) depend on the real code (it's the code that's being tested after all), but real code must never depend on test code.

anselrognlie · 2024-09-29T00:24:36Z

+    all_friends_movies = get_all_friends_movies(user_data)
+
+    for movie in all_friends_movies:
+        if movie["host"] in user_data["subscriptions"]:


A small optimization could be creating a set holding the user subscriptions, then checking whether the host is in the set values.

anselrognlie · 2024-09-29T00:30:18Z

+    for movie in user_data["watched"]:
+        if movie in recommended_movie_list:
+            recommended_movie_list.remove(movie)


Notice that this whole step of filtering out the movies watched by the user is something we did earlier in get_friends_unique_watched. Can we reuse that function to help here?

anselrognlie · 2024-09-29T00:30:59Z

 # -----------------------------------------

+def get_new_rec_by_genre(user_data):
+    favorite_genre = get_most_watched_genre(user_data)


👍 Great reuse of this function. We don't want to duplicate this logic again here!

anselrognlie · 2024-09-29T00:36:49Z

+    for movie in user_data["watched"]:
+        if movie in recommended_genre:
+            recommended_genre.remove(movie)


Could we avoid needing to do this removal if we started with a list of movies the friends watched, but the user hadn't? Can we use a previous wave function to help with this?

anselrognlie · 2024-09-29T00:41:34Z

+    for movie in user_data["favorites"]:
+        if movie not in all_friends_movies:
+            recommended_movies.append(movie)


What if we started with a list of movies the user watched, but the friends hadn't? Can we use a previous wave function to help with this? Could that result be filtered against the movies in the favorite list? If so, consider using a set of favorite movie titles to filter the user movies efficiently. Notice that since we already have other functions that are making new data, this may more clearly help us favor an approach using a set or dict for efficient lookups, even though it would required additional memory. But once we've used some, what's a little more? 😁

anselrognlie

Thanks for updating your submission. There are still a few points about the tests to consider, but overall this looks good!

anselrognlie · 2024-10-18T19:04:16Z

    # Assert
    assert len(updated_data["watchlist"]) == 0
    assert len(updated_data["watched"]) == 1
+    assert MOVIE_TITLE_1 in updated_data["watched"][0].values()


Rather than checking against all values, we'd still want to compare against the value stored under the "title" key

assert MOVIE_TITLE_1 == updated_data["watched"][0]["title"]

But really, we'd want to ensure that all the data is there as expected. To do so , we can make our own dictionary to compare with

assert updated_data["watched"][0] == { "title": MOVIE_TITLE_1, "genre": GENRE_1, "rating": RATING_1 }

And though we know there's only a single movie in the list this time, since the specification doesn't require that movies be moved to a particular location in the watched list, we could use an in check so the location doesn't matter.

expected_movie = { "title": MOVIE_TITLE_1, "genre": GENRE_1, "rating": RATING_1 } assert expected_movie in updated_data["watched"]

anselrognlie · 2024-10-18T19:05:24Z

    # Assert
    assert len(updated_data["watchlist"]) == 1
    assert len(updated_data["watched"]) == 2
+    assert movie_to_watch in updated_data["watched"]


👍 Here, it would also be a good idea to check that the movie we didn't expect to move is still in the watchlist, and the other movie that was already in the watched list is still there.

anselrognlie · 2024-10-18T19:06:46Z


    # Assert
    assert len(friends_unique_movies) == 3
+    assert INTRIGUE_3 in amandas_data["friends"][0]["watched"]


👍 Here, we should also ensure that the other two movies that we'd expect to have reported are also there. Essnetially, the same checks that were done for the previous test (test_friends_unique_movies) would apply here as well.

anselrognlie · 2024-10-18T19:10:55Z

+    assert not recommendations
+    assert len(sonyas_data["watched"]) == 1


👍 Here, the function is specified as returning a list. Though in regular code, we would tend to write if not my_list: to check for an empty list, we can be more strict here to enforce the function requirements by literally comparing to [].

assert recommendations == []

Further, we don't expect the get_new_rec_by_genre function to modify the data structure (in any case), so we don't need to check that the watched list is unchanged.

anselrognlie · 2024-10-18T19:14:24Z

+    movie_dict["title"] = title
+    movie_dict["genre"] = genre
+    movie_dict["rating"] = rating


👍 Yep, this is what we were looking for.

222alle111 and others added 25 commits September 23, 2024 19:03

Test passed for wave1 function1

72d0f96

Test passed for wave1 function2

32fd254

Test passed for wave1 function3

ae7b0d9

uncommented pytest

706e372

Merge branch 'main' of https://github.com/machilula/viewing-party

ddb2c0f

Test wave1 function1 passed changed

9cb00d7

tests wave 1 debugging

87e6878

Merge branch 'main' of https://github.com/machilula/viewing-party

765a9f7

Wave 1 passed

562c7a0

unhashtag

151726d

test merge

54a99d2

Wave2 test passed

7c6919a

wave 3 pre test run

a9b6fe6

Merge branch 'main' of https://github.com/machilula/viewing-party

72f07e0

wave 3 tests passed

ff798d5

Deleted a code

c8dba49

Merge branch 'main' of https://github.com/machilula/viewing-party

2e8bb26

Wave 4 passed

b39f711

wave 4 saves

3a9d2e6

Merge branch 'main' of https://github.com/machilula/viewing-party

6233725

Wave 5 passed

81f223d

wave 5 save

613ee18

refactored edits

283cc18

user_watched deletion

b9bb88b

line 51 indentation fix

612bbce

anselrognlie reviewed Oct 1, 2024

View reviewed changes

updates to all waves

881f00d

anselrognlie reviewed Oct 18, 2024

View reviewed changes

		@@ -1,23 +1,155 @@
		# ------------- WAVE 1 --------------------
		from tests.test_constants import *

		assert not recommendations
		assert len(sonyas_data["watched"]) == 1

Conversation

machilula commented Sep 27, 2024

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants