Sphinx Class_Astry & Rong by cornetto9 · Pull Request #7 · Ada-C22/swap-meet

cornetto9 · 2024-10-03T17:19:38Z

No description provided.

yangashley

Nice job on swap-meet, Astry and Rong!

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-07T17:48:29Z

@@ -1,2 +1,10 @@
-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 👍

yangashley · 2024-10-07T17:55:28Z

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


Nice work on here!

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

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

yangashley · 2024-10-07T17:57:27Z

+
 class Item:
-    pass
+    def __init__(self,id=None, condition=0, age=0):


Suggested change

def __init__(self,id=None, condition=0, age=0):

def __init__(self, id=None, condition=0, age=0):

Nit: You're missing a whitespace after the first comma. Be mindful to use/exclude whitespaces where necessary. It is easy to overlook, but having inconsistent use of whitespaces leads to a messy codebase.

Here's the section on whitespaces in the PEP8 styleguide.

yangashley · 2024-10-07T18:04:55Z

+        self.fabric = fabric
+
+    def __str__(self):
+        return f"An object of type Clothing with id {self.id}. It is made from {self.fabric} fabric."


This method looks similar to the method you wrote in the Item parent class.

In that class __str__ returns f"An object of type Item with id {self.id}."

What if the __str__ method for the Clothing class could inherit this behavior from the parent class and then append the sentence with the second part of the fabric message "It is made from {self.fabric} fabric." ?

Currently, it's not possible because Item's __str__ method hardcodes the classtype "Item" in the string it returns. However, you could refactor the method to make it more dynamic so that it will print any class's name.

After Item's __str__ method is refactored, then Clothing's method could override it like this:

def __str__(self): type_str = super().__str__() fabric_str = f"It is made from {self.fabric} fabric." return " ".join([type_str, fabric_str])

yangashley · 2024-10-07T18:06:19Z

+        self.width = width
+        self.length = length
+
+    def __str__(self):


See comment from clothing.py about refactoring this method so it overrides the parent class's __str__ method.

yangashley · 2024-10-07T19:19:59Z

+    def get_newest_item(self):
+        newest_item = self.inventory[0]
+
+        for item in self.inventory:
+            if item.age < newest_item.age:
+                newest_item = item
+
+        return newest_item 
+
+    def swap_by_newest(self, other_vendor):
+        my_newest_item = self.get_newest_item()
+        their_newest_item = other_vendor.get_newest_item()
+
+        return self.swap_items(other_vendor, my_newest_item, their_newest_item)


yangashley · 2024-10-07T19:21:27Z

+    assert len(fatimah.inventory) == 3
+    assert item_b in fatimah.inventory
+    assert item_a not in fatimah.inventory
+    assert item_c in fatimah.inventory
+    assert item_d in fatimah.inventory
+    assert len(jolie.inventory) == 2
+    assert item_d not in jolie.inventory
+    assert item_e in jolie.inventory
+    assert item_a in jolie.inventory
+    assert result


LGTM! Gad you got some additional practice with this optional enhancement!

yangashley · 2024-10-07T19:23:24Z

-    # *********************************************************************
-    # ****** Complete Assert Portion of this test **********
-    # *********************************************************************
+    assert result == False


Since the general intention of this method is to modify the inventory, we want to check this condition as well to make sure that the items that started in the inventory are still there. Adding these assertions would make the test more robust.

assert len(vendor.inventory) == 3 assert "a" in vendor.inventory assert "b" in vendor.inventory assert "c" in vendor.inventory

yangashley · 2024-10-07T19:27:46Z

-    # *********************************************************************
+    assert len(jolie.inventory) == 0
+    assert len(fatimah.inventory) == 3
+    assert not result


More than be falsy, swap_items is intended to return an actual boolean. So while we typically write something like

assert not result

in regular python, here we might prefer the more explicit check of

assert result == False

You could make this test more robust by verifying that the items in the unchanged inventory are actually the items we started with (and not some other 3 items)

assert item_a in fatimah.inventory assert item_b in fatimah.inventory assert item_c in fatimah.inventory

yangashley · 2024-10-07T19:39:19Z

+    assert item_f not in jesse.inventory
+    assert item_c not in tai.inventory


We don't need to include the not in checks since the other checks already rule them out.

SpectreKitty and others added 20 commits September 30, 2024 15:13

wave 1 complete with all wave 1 tests passing

3dd47c0

add wave 2

1325e86

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

c6954f9

add wave 2 item class

7ff7d4a

preemptively adding in method info for vendor

9d1cf6d

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

24ca3c2

Wave 2 completed with tests passing

b424232

Wave 3 vendor class with relevant tests passing

6d8a5cd

Wave 4 completed with tests passing

e5098c1

add wave 3 str() in Item class

007f858

add wave 5 clothing & decor

5549cb2

Wave 5 completed and tests passing

1bdb0d4

add wave 6 1 & 2 function

eed1006

all waves completed and all tests passing

51276d6

refactored swap_first_item func all tests still passing

78449fb

update and add optional

b0317b8

add notes to self.__class__.__name__

cac7fc2

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

7f6391b

resolved merge conflict and made small spacing changes

01be4fb

minor comments update

f6aca3d

yangashley reviewed Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sphinx Class_Astry & Rong#7

Sphinx Class_Astry & Rong#7
cornetto9 wants to merge 20 commits into
Ada-C22:mainfrom
cornetto9:main

cornetto9 commented Oct 3, 2024

Uh oh!

yangashley left a comment

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

yangashley Oct 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

	def __init__(self,id=None, condition=0, age=0):
	def __init__(self, id=None, condition=0, age=0):

		assert item_f not in jesse.inventory
		assert item_c not in tai.inventory

Conversation

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants