-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add dictionary merge solution and related test #34
Add dictionary merge solution and related test #34
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Frank, these are the stuff that were missing from the challenge and the tests file, they are the boxes that I didn’t tick in the checklist as well
import unittest | ||
from solutions.merge_dictionaries import merge_dictionaries | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a module docstring to the Tests file
from solutions.merge_dictionaries import merge_dictionaries | ||
|
||
|
||
class TestMergeDictionaries(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test class should have the name in PascalCase
You should add a docstring to the test class
|
||
|
||
class TestMergeDictionaries(unittest.TestCase): | ||
def test_no_conflicts(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add docstrings to each unit test that describes what the test is checking
dict2 = {"a": 2} | ||
expected = {"a": 2} | ||
self.assertEqual(merge_dictionaries(dict1, dict2), expected) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add defensive assertions tests ( Tests that focus on verifying that a function has internal checks (like assertions) to catch invalid states or inputs )
solutions/merge_dictionaries.py
Outdated
dict_a = {"x": 1, "y": 2} | ||
dict_b = {"y": 3, "z": 4} | ||
|
||
print("Default merge (dict_b overwrites):") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should NOT have print statements anywhere
>>> merge_dictionaries({'a': 1}, {'a': 2, 'b': 3}) | ||
{'a': 2, 'b': 3} | ||
>>> merge_dictionaries({'a': 1}, {'a': 2, 'b': 3}, max) | ||
{'a': 2, 'b': 3} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add more doctests ( you only have 2, there should be at least 3 )
{'a': 2, 'b': 3} | ||
>>> merge_dictionaries({'a': 1}, {'a': 2, 'b': 3}, max) | ||
{'a': 2, 'b': 3} | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add defensive assertions and they should include as little logic as possible
You should document defensive assertions using Raises:
- Each assumption about an argument must be checked with an assertion
- Each assertion should check for only 1 assumption about the argument
I can see that you added all the changes and edits required, good job!! It's ready to merge now |
name: solution review
about: A template PR for code review with a checklist
Behavior
Files
/tests/test_file_name.py
Unit Tests
Function Docstring
Raises:
The Function
Strategy
Do's
Don'ts
Implementation
when it's too restricting.
print
statements anywhere