-
Notifications
You must be signed in to change notification settings - Fork 32
mailroom app with basic logging #187
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.
Logging, and tests and packaging, oh, my. Looks great. I didn't run the tests or the code, but everything looks good. I didn't see any configuration for logging, so you should probably add that (unless I just missed it somehow). Good to know how to change the defaults, even if you don't care about logging to a file in this particular case.
|
|
||
|
|
||
| def send_thank_you(): | ||
| """ Record a donation and send thank you message """ |
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.
I would consider breaking this into two functions, get_donor_name and get_donation_amount, or something like that. Your function would look something like this then:
def send_thank_you():
donor = get_donor_name() # this function would run the while loop, add the donor to the database if it isn't there and return the name
amount = get_donation_amount() # this function would run the second while loop, get the donation amount
donor.add_donation(amount)
print(DB.send_letter(donor))
| def __init__(self, donor_name, initial_donations=None): | ||
| logging.info("Set up donor '%s'", donor_name) | ||
| self.name = donor_name | ||
| if initial_donations is None: |
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.
Preferred way of saying this is if not initial_donations:
| return self.donations[-1] | ||
| except IndexError: | ||
| logging.warn("Donor %s has not made any donations", self.name) | ||
| return None |
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.
Python returns None by default.
| user_input = ["Matt", "100"] | ||
| with mock.patch("builtins.input", side_effect=user_input): | ||
| cli.send_thank_you() | ||
| assert mock_stdout.getvalue() == THANK_YOU_OUTPUT |
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.
Looks like you got sys.stdout patched! I think there were others having difficulties with this, so you might want to post a link to your tests in slack so others can see what you did.
No description provided.