TASK: Add Inventory system (#65)#71
Conversation
TheGittyPerson
added
feature
There was a problem hiding this comment.
Thanks for your PR! These are very well-written modules and I see no immediate errors or formatting issues. However here are just some small changes to error messages I recommend before I start analysing the code in more detail with some testing, which will take more time, so here are the minor fixes for now. (I didn't add comments in item.py as it's the same fixes)
Before we continue though, add a new Person object the_town.py (you can name it after yourself if you wish) that uses the Inventory class. In other words, create a Person instance and write some example code that shows how the Inventory methods and features are used in practice. I’ll also add some examples later, but since you’re currently more familiar with this new feature, I’d like you to create the initial demonstration first.
Thanks! Please do ask if you have any questions
| ValueError: If max_capacity is negative. | ||
| """ | ||
| if not isinstance(max_capacity, int): | ||
| raise TypeError("max_capacity must be an integer.") |
There was a problem hiding this comment.
Avoid using periods in error messages (to stay consistent with the rest of the code). You should also put quotes around varibale names. So this could be:
f"'max_capacity' must be an integer, got type '{type(max_capacity)}' instead"| if not isinstance(max_capacity, int): | ||
| raise TypeError("max_capacity must be an integer.") | ||
| if max_capacity < 0: | ||
| raise ValueError("max_capacity must be non-negative.") |
There was a problem hiding this comment.
Same here — this could be:
f"'max_capacity' must be non-negative'"| ValueError: If adding the item exceeds max capacity. | ||
| """ | ||
| if not isinstance(item, Item): | ||
| raise TypeError("item must be an Item.") |
There was a problem hiding this comment.
"'item' must be an 'Item' object"| if not isinstance(item, Item): | ||
| raise TypeError("item must be an Item.") | ||
| if not isinstance(quantity, int): | ||
| raise TypeError("quantity must be an integer.") |
There was a problem hiding this comment.
"'quantity' must be an integer"| if not isinstance(quantity, int): | ||
| raise TypeError("quantity must be an integer.") | ||
| if quantity <= 0: | ||
| raise ValueError("Quantity must be positive.") |
There was a problem hiding this comment.
"'quantity' must be positive"| if quantity <= 0: | ||
| raise ValueError("Quantity must be positive.") | ||
| if self.item_count + quantity > self.max_capacity: | ||
| raise ValueError("Inventory capacity exceeded.") |
There was a problem hiding this comment.
just remove the period here
| raise TypeError("item must be an Item.") | ||
| if not isinstance(quantity, int): | ||
| raise TypeError("quantity must be an integer.") | ||
| if quantity <= 0: | ||
| raise ValueError("Quantity must be positive.") | ||
|
|
||
| if item.stackable: | ||
| if item not in self.stackable_items: | ||
| raise ValueError("Item not found in inventory.") | ||
| if self.stackable_items[item] < quantity: | ||
| raise ValueError("Not enough quantity to remove.") |
There was a problem hiding this comment.
Same here for these error messages
| self, | ||
| name: str, | ||
| calories: int, | ||
| cooked: bool = False, |
There was a problem hiding this comment.
Allow None, as some food aren't meant to be cooked (like potato chips or coffee)
ilmya
force-pushed
the
feature/inventory-system
branch
from
f64ccc2 to
93b5914
Compare
TheGittyPerson
force-pushed
the
feature/inventory-system
branch
2 times, most recently
from
82e5ed4 to
47cc6d6
Compare
|
I reset your branch back and cherry-picked your commits so that there are no merge commits 👍 |
ilmya
force-pushed
the
feature/inventory-system
branch
from
47cc6d6 to
52ffdcf
Compare
|
I’ve applied the suggested changes to the error messages. |
Thanks a lot for cleaning that up! I did struggle a bit with getting the history right, so this really helps. |
Install `pytest` and run the `pytest` command from project root to run the tests. Code generated by Codex Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
|
I added a test file for your module. You can try it out if you're curious (install |
- Added immutability enforcement on `Item` with test coverage (`test_item_is_immutable`). - Introduced clone methods on `Item` and `Inventory` to support safe mutations. - Updated `Item.value` to allow `None` and adjusted `Inventory.total_value` to handle it safely. Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
Signed-off-by: Morpheus <167074500+thegittyperson@users.noreply.github.com>
good job bro nice |
3 participants
Summary
This PR adds an inventory system to
theperson.Changes
Inventory system
inventory.pymodule with anInventoryclass intheperson/add_itemremove_itemclearlist_itemsis_emptyhas_itemget_quantityis_fullremove_allfilter_itemssort_itemstotal_valueInventory state/configuration
item_countas a state propertyunique_item_typesas an additional state propertymax_capacityas a configuration valueItem module
item.pymodule with anItembase class intheperson/namevaluestackableFoodincludescalories,cooked, andbrandElectronicincludesbrandandbatteryValuableWeaponOthers
Personnow initializes with anInventoryinstance by defaultTesting
Manually tested:
Test code can be provided if needed.
Notes
This implementation uses a hybrid inventory structure:
At this stage, I did not add extra methods for inventory operations, as the functionality is already accessible via
person.inventory. This does not prevent such methods from being added later if needed.For the same reason,
person.pydoes not importItemdirectly, since it is not used there yet. I avoided adding an unused import or formality, especially to keep the code clean and pylint-compliant.The current design is intentionally kept simple. Further improvements or more advanced features can be explored in future updates or separate issues.
Closes #65