Sphinx- Carla & Denise by dpchengmath · Pull Request #5 · Ada-C22/swap-meet

dpchengmath · 2024-10-03T16:07:20Z

No description provided.

…e 1 Unit Tests

yangashley

Nice job on swap-meet, Carla and Denise!

I can see from the commit history that you made frequent commits, which is great!

Let me know if you have any questions about my comments.

yangashley · 2024-10-08T16:38:27Z

swap_meet/clothing.py

@@ -1,2 +1,15 @@
-class Clothing:
-    pass
+from swap_meet.item import Item


Nice work importing Item since we're using by name as the parent class of Clothing 👍

Nit: Inconsistent use of double and single quotes in this file. Pick one and stick to it for a project.

✅ Fixed (will push later!)

yangashley · 2024-10-08T16:39:19Z

swap_meet/clothing.py

+    def __init__(self, id=None, fabric="Unknown", condition=0, age=0):
+        super().__init__(id, condition, age)


Nice work on here!

On line 4 you correctly set the default value for fabric to "Unknown".

On line 5 you properly call the constructor of the parent class, and pass along the values the child wants to use.

Lastly, good work remembering to refactor the function definition to include age so that it can get set from the subclasses.

✅ Thanks!

yangashley · 2024-10-08T16:41:11Z

swap_meet/clothing.py

+        self.fabric = fabric
+
+    def get_category (self):
+        return "Clothing"


get_category should only be written once in the parent class (Item). The child class should be able to invoke the parent class's get_category method and have it return "Clothing" without overriding the method here and hardcoding some value.

Which of these is better? They both worked with the tests:

return type(self).__name__ return self.__class__.__name__

What do you think?

They do both work, but there might be some opinions to be had about which is better. Have a look online, think you'll find some informative threads on SO.

yangashley · 2024-10-08T16:42:08Z

swap_meet/clothing.py

+        return (
+            f'An object of type Clothing with id {self.id}. '
+            f'It is made from {self.fabric} fabric.'


While this method overrides the parent class's __str__ method, it repeats logic that is already written. In the parent class (Item) you already wrote "An object of type Item with id {self.id}." We should refactor the parent method so more of the logic can be re-used here.

I would expect this overriden function to look something like this:

def __str__(self): return f"{super().__str__()} It is made from {self.fabric} fabric."

✅ Fixed (will push later!)

yangashley · 2024-10-08T16:44:14Z

swap_meet/decor.py

+    def get_category (self):
+        return "Decor"
+
+    def __str__(self):
+        return (
+            f'An object of type Decor with id {self.id}. '
+            f'It takes up a {self.width} by {self.length} sized space.'
+            )


See comment above in clothing.py about not needing to override get_category just to hardcode a string value. We should be able to dynamically get this value when the parent class's method is invoked.

Also, same comment from above applies here about __str__. We should not need to hardcode "Decor" in the string and repeat logic that was written in the parent class.

These comments apply to Electronic class's instance methods of the same names too.

✅ Fixed (will push later!)

yangashley · 2024-10-08T18:50:30Z

tests/unit_tests/test_wave_06.py

+    assert item_c not in tai.inventory
+    assert item_c in jesse.inventory
+    assert item_f not in jesse.inventory
+    assert item_f in tai.inventory
+    assert item_b not in jesse.inventory
+    assert item_a in tai.inventory
+    assert item_b in tai.inventory
+    assert item_d in jesse.inventory


✅ Thanks!

yangashley · 2024-10-08T18:51:01Z

tests/unit_tests/test_wave_06.py

-    # - That result is truthy
-    # - That tai and jesse's inventories are the correct length
-    # - That all the correct items are in tai and jesse's inventories, and that the items that were swapped are not there
+    assert len(jesse.inventory) == len(tai.inventory)


See comment above. Is this test concerned with knowing that the length of these two lists are the same? Is that part of the logic of the method that is under test here?

Same comment applies to similar assertions written in the tests below.

✅ Fixed (will push later!)

yangashley · 2024-10-08T18:51:34Z

tests/unit_tests/test_wave_06.py

+    assert item_a not in jesse.inventory
+    assert item_d in jesse.inventory
+    assert item_d not in tai.inventory
+    assert result


See comment above about explicitly testing for the value of result instead of checking that result is a truthy value.

✅ Fixed (will push later!)

yangashley · 2024-10-08T18:53:30Z

tests/unit_tests/test_wave_06.py

+    assert item_a not in jesse.inventory
+    assert item_c in tai.inventory
+    assert item_c not in jesse.inventory
+    assert result == False


👍 nice job explicitly checking that result is False and not just a falsey value.

✅ Thanks!

yangashley · 2024-10-08T18:55:45Z

tests/unit_tests/test_wave_07.py

This file appears to be identical to the file that was part of the template repo before you added your changes in. Was this change accidentally committed?

If this change wasn't needed then take care to remove it from the list of files being added and committed. You can unstage a file for commit if you haven't run git commit yet and you can look at how to locally revert a commit if you already added/committed the file.

In industry, when you write code and open a PR it means you're asking to merge code into the main branch. Therefore, you will not be permitted to merge in changes that are not part of your feature work because it will cause noise in the commit history or bring in unnecessary changes to the codebase.

It's best practice to run git status to see what changes will be committed before you commit so you can catch stray changes like this.

We created this file test_wave_07 to test for the optional enhancements from the readme since it didn't exist yet (we created the .py test and modified another test template in another wave.) Was that correct? It was to test that our methods, attributes, and functions swap_best_by_category and swap_newest_items. Let us know if this works well or if we should change. Thanks!

ohhhhh you know what, I'm a ding dong and I diffed the file against itself so I was completely wrong and thought that it was something already in the code base. Let me review it! Thanks for explaining

yangashley · 2024-10-09T17:26:26Z

tests/unit_tests/test_wave_07.py

+    result = fatimah.swap_by_newest(jolie)
+
+    with pytest.raises(ValueError):
+        if result == False:
+            raise ValueError(f"An exception occurred.")


When you use pytest.raises() in a unit test, the function under test (swap_by_newest), should be in the codeblock that follows pytest.raises().

You invoke the method then check if result == False which does not rely on the tool to check that the method you wrote is throwing an error. You're checking if some value is equal to something else and then manually raising an error in the test on line 98.

When writing a test to see if the function under test is correctly raising an error for given invalid inputs, we should not be manually raising errors in tests. This is forcing the test to pass by saying "ok I need an error, I'm gonna make an error happen in the test by raising it". What we should be asking ourselves is "Will my function raise the error I think it does when I pass it specific inputs?"

This test should look like

def test_swap_newest_items_from_my_empty_returns_false(): fatimah = Vendor( inventory=[] ) item_d = Item() item_e = Item() jolie = Vendor( inventory=[item_d, item_e] ) with pytest.raises(ValueError): fatimah.swap_by_newest(jolie)

Review these resources for more details: https://docs.pytest.org/en/stable/how-to/assert.html#assertraises and https://learn-2.galvanize.com/cohorts/4211/blocks/1032/content_files/exception-handling/checking-exceptions-in-tests.md

yangashley · 2024-10-09T17:27:33Z

tests/unit_tests/test_wave_07.py

+    result = fatimah.swap_by_newest(jolie)
+
+    with pytest.raises(ValueError):
+        if result == False:
+            raise ValueError(f"An exception occurred.")


See comment above about invoking the method under test in the body of pytest.raises()

yangashley · 2024-10-09T17:28:13Z

tests/unit_tests/test_wave_07.py

Tests here look good. I did call out incorrect use of pytest.raises so please review and make sure that is clear. Let me know if you have questions!

yangashley · 2024-10-09T17:31:37Z

swap_meet/vendor.py

+        if  first_item  and first_item_other:
+            my_item_index = self.inventory.index(first_item)
+            their_item_index = other_vendor.inventory.index(first_item_other)
+            other_vendor.inventory[their_item_index], self.inventory[my_item_index] = first_item, first_item_other
+            return True


Nit on line 46 about white spaces:

Suggested change

if first_item and first_item_other:

my_item_index = self.inventory.index(first_item)

their_item_index = other_vendor.inventory.index(first_item_other)

other_vendor.inventory[their_item_index], self.inventory[my_item_index] = first_item, first_item_other

return True

if first_item and first_item_other:

my_item_index = self.inventory.index(first_item)

their_item_index = other_vendor.inventory.index(first_item_other)

other_vendor.inventory[their_item_index], self.inventory[my_item_index] = first_item, first_item_other

return True

Also, do we need to write the logic on lines 47-50? Could we invoke swap_items instead?

def swap_first_item(self, other_vendor): if not self.inventory or not other_vendor.inventory: return False first_item = self.inventory[0] first_item_other = other_vendor.inventory[0] if first_item and first_item_other: return swap_items(other_vendor, first_item, first_item_other)

yangashley · 2024-10-09T17:33:13Z

swap_meet/item_helper_functions.py

Nice, exactly what I meant by moving them out of the Vendor class

yangashley · 2024-10-09T17:33:50Z

swap_meet/vendor.py

+from swap_meet.item_helper_functions import get_highest_item
+from swap_meet.item_helper_functions import get_newest_item


You can actually import multiple objects from the same module in one line like:

from swap_meet.item_helper_functions import get_highest_item, get_newest_item

dpchengmath and others added 30 commits September 30, 2024 16:28

Wave 1 completed with all tests passing and asserts completed for Wav…

0245b02

…e 1 Unit Tests

"Passes all tests of wave 2"

23ce217

"Worked on wave 1"

f3f6d14

"Nothing to commit"

e14241c

collab work on wave 3/ not completed yet

c0cc7bb

collab work on wave 3... still not complete

f10dd5c

"No changes"

8794a06

"No changes"

45c515f

"Corrects Item file"

5fca3b1

"Fixed error in tests"

e3bd7bf

"Passes tests of wave 3"

74b28b6

"Passes all tests of wave 4"

9958170

"Passes all tests of no. 5"

e202fe1

"Wave 5 tests all passing"

e92e16b

Merge branch 'main' of https://github.com/dpchengmath/swap-meet

472a851

"Passes all the tests"

cce99df

"Fixes formatting"

e7acbf0

Drying & cleaning up swap meet

6354bde

String concatenation in swap meet

1187900

Working on wave 7 implementation. This version is not complete yet.

bf07ddd

Nothing to commit

be1eaea

"Fixes errors"

7a1ebe0

"Passes first test wave 7"

54bdbfd

"Creates a new test and passes it"

88152bf

Updating back to last commit with passing tests.

d131d68

Merge branch 'main' of https://github.com/dpchengmath/swap-meet

9bfae7d

"Creates new tests for wave 7 and fixes formatting"

5db5621

"Checks one more formatting"

a909b0a

"Fixes test after last commit changed it to have an error"

be231f3

"Fixes test 5"

8c00705

dpchengmath and others added 5 commits October 2, 2024 20:32

All waves 1-7 completed with passing tests and clean format

d5002ee

"Creates a function to not use built-in max function"

bd6ff1d

Creates a function to not use built-in max function

51cac06

Creates a function to not use built-in min function

1421096

Merge branch 'main' of https://github.com/dpchengmath/swap-meet

2dc6672

yangashley reviewed Oct 8, 2024

View reviewed changes

dpchengmath added 5 commits October 8, 2024 21:07

Improvements to swap meet project

bb270d8

Created item helper functions and cleaned up

e00639b

Edits to swap meet project for all waves and tests

306d588

Cleaning up swap meet

5f324b6

Cleaning up swap meet

514e1c4

yangashley reviewed Oct 9, 2024

View reviewed changes

swap_meet/item_helper_functions.py

Copy link

yangashley Oct 9, 2024

Choose a reason for hiding this comment

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

Nice, exactly what I meant by moving them out of the Vendor class

yangashley reviewed Oct 9, 2024

View reviewed changes

		def __init__(self, id=None, fabric="Unknown", condition=0, age=0):
		super().__init__(id, condition, age)

		from swap_meet.item_helper_functions import get_highest_item
		from swap_meet.item_helper_functions import get_newest_item

Conversation

dpchengmath commented Oct 3, 2024

Uh oh!

yangashley 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!

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!

Choose a reason for hiding this comment

Uh oh!

yangashley Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yangashley Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

yangashley Oct 9, 2024 •

edited

Loading

yangashley Oct 9, 2024 •

edited

Loading