Skip to content

Amethyst - Elaine W.#91

Open
ewatki wants to merge 35 commits into
Ada-C19:mainfrom
ewatki:main
Open

Amethyst - Elaine W.#91
ewatki wants to merge 35 commits into
Ada-C19:mainfrom
ewatki:main

Conversation

@ewatki

@ewatki ewatki commented Apr 24, 2023

Copy link
Copy Markdown

No description provided.

ewatki and others added 30 commits April 3, 2023 13:12

@audreyandoy audreyandoy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work Elaine and Raina! You hit all the learning goals for this project and all tests are passing. You have earned a well-deserved 🟢 grade on this project ✨

I added comments, compliments, and hints on ways to refactor your code.

Keep up the great work! ✨

Comment thread README.md
- Note that this package's functions return `UUID` objects, not integers as such, **but** `UUID` objects have [an attribute `int`](https://docs.python.org/3/library/uuid.html#uuid.UUID.int) which allow us to access their value as an integer
- When we initialize an instance of `Item`, we can optionally pass in an integer with the keyword argument `id` to manually set the `Item`'s `id`
- Each `Item` will have a function named `get_category`, which will return a string holding the name of the class
- Each `Item` will have a function named `get_category`, which will return a string holding the name of the class (named Vendor instance, ex: "Clothing", "Decor", "Electronics ")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like this clarification!

Comment thread swap_meet/clothing.py
@@ -1,2 +1,13 @@
class Clothing:
pass No newline at end of file
# import uuid

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We can remove commented-out code in future PR's.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In industry, PR's are used to present a solution or code added to a project shared with the rest of the team with the intention of eventually being reviewed and merged into production. Commented out code isn't going to be merged into production (at least, that's best practice) so we can remove it from PRs (:

Comment thread swap_meet/clothing.py
Comment on lines +5 to +12
class Clothing(Item):
def __init__(self, id = None, fabric="Unknown", condition = 0):
super().__init__(id, condition)
# self.id = id
self.fabric = fabric

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Great work using the parent constructor to create the id and condition attributes.

Comment thread swap_meet/decor.py
Comment on lines +5 to +13
class Decor(Item):
def __init__(self, id = None, width = 0, length = 0, condition = 0):
super().__init__(id, condition)
# self.id = id
self.width = width
self.length = length

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread swap_meet/electronics.py
Comment on lines +5 to +13
class Electronics(Item):
def __init__(self, id = None, type = "Unknown", condition = 0):
super().__init__(id, condition)
# self.id = id
self.type = type


def __str__(self):
return f"An object of type Electronics with id {self.id}. This is a {self.type} device."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 We can remove self.id = id since it's commented out.

Comment on lines +134 to +136
assert len(fatimah.inventory) == 3
assert len(jolie.inventory) == 0
assert not result

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment on lines +112 to +115
assert result
assert len(tai.inventory) == len(jesse.inventory)
assert item_f not in jesse.inventory
assert item_e not in tai.inventory

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment on lines +142 to +144
assert result
assert len(tai.inventory) == len(jesse.inventory)
assert (item_d and item_f) in tai.inventory and (item_d and item_f) not in jesse.inventory

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Nitpick: if I'm testing 2 different data structures (tai's inventory and jesse's inventory) I prefer to have separate asserts just to make thediff message from the failed test easier to read.

This can be applied to the rest of the tests.

Comment on lines +226 to +227
assert jesse
assert tai

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These asserts only check if jesse and tai are truthy, which means that as long as jesse and tai have items within their lists (or is any truthy value like an integer, string, etc.) then the test will pass. We may want to be more explicit to ensure that jesse and tai's inventory lists remain unchanged.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

example:

assert tai.inventory == [item_a, item_b, item_c]
assert jesse.inventory == [item_d, item_e, item_f]

Comment on lines +256 to +260
assert not result
assert len(tai.inventory) == 3
assert len(jesse.inventory) == 3
assert jesse
assert tai

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See comment above.

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.

3 participants