Conversation
… unique attributes (fabric, type, or space used). Created a new test file test_wave_enhancements.
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Lisa! I've left a mix of suggestions and questions to consider for feedback. Please reply here on Github or reach out on Slack if there's anything I can clarify =]
| @@ -0,0 +1,73 @@ | |||
| {\rtf1\ansi\ansicpg1252\cocoartf2639 | |||
There was a problem hiding this comment.
Was this file meant to be committed?
| self.type = type | ||
|
|
||
| def __str__(self): | ||
| return f"{super().__str__()}. This is a {self.type} device." No newline at end of file |
There was a problem hiding this comment.
Really nice use of inheritance across the child classes 👍
| if not id: | ||
| self.id = uuid.uuid4().int | ||
| else: | ||
| self.id = id |
There was a problem hiding this comment.
Nice solution, another option here could use conditional assignment:
# Assigns self.id to the parameter id if id is truthy,
# otherwise assigns self.id the return value of uuid.uuid4().int
self.id = id if id else uuid.uuid4().int| return "Fair" | ||
| elif 3 < self.condition <= 4: | ||
| return "Good" | ||
| elif 4 < self.condition <= 5: |
There was a problem hiding this comment.
I like the use of ranges to account for decimal condition values.
| @@ -1,2 +1,146 @@ | |||
| from .item import Item | |||
There was a problem hiding this comment.
If we don't use a class name to do something like create a new instance or call a class method, we likely don't need to import it and should remove the import line.
| my_item = self.get_by_id(my_item_id) | ||
| their_item = other_vendor.get_by_id(vendor_item_id) | ||
|
|
||
| if my_item and their_item: | ||
| return self.swap_items(other_vendor, my_item, their_item) |
There was a problem hiding this comment.
Another option could be to use the swap_by_id function here.
| @pytest.mark.skip | ||
| @pytest.mark.integration_test | ||
| #@pytest.mark.skip | ||
| #@pytest.mark.integration_test |
There was a problem hiding this comment.
For integration tests, we want to leave the decorator @pytest.mark.integration_test uncommented. This decorator tells pytest that this is an integration test, so we should run it after all of the unit tests.
| result = vendor.remove(item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result == None |
There was a problem hiding this comment.
Nice assert, we definitely want to confirm our return value is None. We generally also want to confirm that there weren't side affects or changes to the input that we didn't intend. What could we assert about the vendor's inventory to confirm that its content didn’t change?
| result = fatimah.swap_items(jolie, item_b, nobodys_item) | ||
|
|
||
| raise Exception("Complete this test according to comments below.") | ||
| assert result == False |
There was a problem hiding this comment.
The feedback in wave 1 applies here as well, what else would be helpful to assert to ensure there were no unintended side effects?
| assert item_c in vendor1.inventory | ||
| assert item_d in vendor2.inventory | ||
| assert item_e in vendor2.inventory | ||
| assert item_f in vendor2.inventory No newline at end of file |
2 participants
No description provided.