Skip to content
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

PR request 12-15-2018 #220

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

CHENGLIUBI
Copy link
Contributor

Updated mailroom oo for final review, but will continue working on it if time allowed. Thank you!

@CHENGLIUBI
Copy link
Contributor Author

Updated mailroom_final.py, mailroom_final_test.py under folder session09

pytest success except add new donor; still need to improve the report & file output pieces.

Please review for final grading. I will continue working on it and submit a new PR request when ready. Thank you very much!

@@ -0,0 +1,85 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

you can not have files named """ -- I"m guessing it's a mistake, but make sure not to add them to git!

it's not so bad in OS-X and Linuc, but breaks Windows :-)

please remove this with:

git rm """.py

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait! sorry about that, this isn't your file -- it was added by another student -- I had removed it, but not sure how you got it back -- I'll clean it up.

Copy link
Contributor

@PythonCHB PythonCHB left a comment

Choose a reason for hiding this comment

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

nice OO structure.

Do make sure that the exception you are catching is actually raised!

@@ -0,0 +1,85 @@
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait! sorry about that, this isn't your file -- it was added by another student -- I had removed it, but not sure how you got it back -- I'll clean it up.



# def test_report():
# assert "Jeff Bzo" in report
Copy link
Contributor

Choose a reason for hiding this comment

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

nice tests!

self.donations = list(donations) # converting donations from tuple to list

@property
def donor(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?? your Donor class holds this info ...

return (self.name, self.donations)

@property
def donor_name(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just use a_donor.name ? The property isn't adding any functionality.

def list_donations(self):
try:
return self.donations
except IndexError:
Copy link
Contributor

Choose a reason for hiding this comment

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

how would you get an IndexError from returning self.donations ??

def total_donations(self):
try:
return sum(self.donations)
except IndexError:
Copy link
Contributor

Choose a reason for hiding this comment

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

sum() will not return an IndexError -- it will return zero for an empty list, though :-)

def num_of_donations(self):
try:
return len(self.donations)
except IndexError:
Copy link
Contributor

Choose a reason for hiding this comment

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

len() wont raise an IndexError either.


def find_donor(self, name): # find donations based on donor name
if name in self.donor_data.keys():
return self.donor_data.get(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are using .get(), there is not need to check if it's there first.

return None

# need more works here
# def add_donor(self, donors):
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean this to be:

def add_donor(self, name):
   ...


def list_donors(self):
current_donors = []
for data in self.donor_data: # donor_data is a dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

nice place for a list comprehension here...

@@ -1,27 +0,0 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

don't delete anything outside your own students DIR!

you really need to read the git commit messages to make sure that you really want to do what you are doing.

@PythonCHB
Copy link
Contributor

I won't merge, unless you restore the file in notes_for_class -- but nice work, you've passed :-)

@PythonCHB PythonCHB added git Problem not merging due to git issue Credit Given Credit given in Canvas -- code not reviewed labels Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Credit Given Credit given in Canvas -- code not reviewed git Problem not merging due to git issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants