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

Niambi P - Sapphire #88

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

Niambi P - Sapphire #88

wants to merge 11 commits into from

Conversation

Lemmi-C
Copy link

@Lemmi-C Lemmi-C commented Apr 7, 2023

No description provided.

Copy link

@mmcknett-ada mmcknett-ada left a comment

Choose a reason for hiding this comment

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

Green 🟢!

The integration tests that were skipped and failing were because of a known Python gotcha with = [] as a default param.

return f"An object of type {self.get_category()} with id {self.id}."

def condition_description(self):

Choose a reason for hiding this comment

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

Be careful about indentation! The only reason that you had to copy/paste this function into your Decor, Clothing, and Electronics classes is the missing indentation before this function. If you had indented here, it would have made condition_description a member of Item, and this function would automatically have been inherited by all the other derived classes.

Comment on lines +15 to +16
return "Clothing"

Choose a reason for hiding this comment

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

None of your derived classes need to implement get_category because you have implemented it in their base class (Item)!

@@ -4,6 +4,7 @@

@pytest.mark.skip

Choose a reason for hiding this comment

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

You should remove the skips on your integration tests. If you do, you'll see there are some errors. See my comment in vendor.py for what's going on there.

Comment on lines +7 to +8
self.inventory = inventory #each vendor will have an attribute named :inventory (an empty list)

Choose a reason for hiding this comment

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

This is what's failing your integration tests. In Python, when you assign a "mutable" object like a list as a default value, every time the function uses that default value it will share the same instance of that object. The workaround is to use None as a default for lists and check for None in the initializer:

    def __init__(self, inventory = None):
        self.inventory = inventory if inventory else []

^-- if you do this instead, your integration tests will pass (after you unskip them).

Comment on lines +7 to +11
self.id = uuid.uuid4().int
#if id is manually assigned
else:
self.id = id

Choose a reason for hiding this comment

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

A ternary is also fine here:

        self.id = id if id else uuid.uuid4().int

Comment on lines +49 to +52
self.remove(self.inventory[0])
self.add(other_vendor.inventory[0])
other_vendor.remove(other_vendor.inventory[0])

Choose a reason for hiding this comment

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

[nit] This looks a lot like swap_items. You could call self.swap_items instead of rewriting very similar logic. 😄

Comment on lines +56 to +61

for item in self.inventory:
if item.get_category() == category:
objects.append(item)
return objects

Choose a reason for hiding this comment

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

Notice this pattern of:

result_list = []
for element in source_list:
    if some_condition(element):
        result_list.append(element)

can be rewritten using a list comprehension as:

result_list = [element for element in source_list if some_condition(element)]

Which here would look like:

items_in_category = [item for item in self.inventory if item.get_category() == category]
return items_in_category

At first, this may seem more difficult to read, but comprehensions are a very common python style, so I encourage y’all to try working with them!

Comment on lines +69 to +73
for item in items:
if item.condition >= highest_condition.condition:
highest_condition = item
return highest_condition

Choose a reason for hiding this comment

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

Great job using loops for this! Once you're comfortable with them, you can do this same sort of thing with the max function:

return max(items, key=lambda item: item.condition)

Comment on lines +80 to +84
return False
else:
self.swap_items(other_vendor, my_best_item, their_best_item)
return True

Choose a reason for hiding this comment

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

[suggestion] You could also simply return self.swap_items(other_vendor, my_best_item, their_best_item), since swap_items handles None.

Comment on lines +110 to +120
assert result == True
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert item_a in tai.inventory
assert item_b in tai.inventory
assert item_c in jesse.inventory
assert item_d in jesse.inventory
assert item_e in jesse.inventory
assert item_f in tai.inventory
assert item_c not in tai.inventory
assert item_f not in jesse.inventory

Choose a reason for hiding this comment

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

Good tests, very thorough.

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