-
Notifications
You must be signed in to change notification settings - Fork 79
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
Tigers - Pavi and Rose D18 #38
base: main
Are you sure you want to change the base?
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.
Pavi & Rose, what you have here is pretty good. It's clear you didn't have time to finish, but I left some comments and suggestions which may help with future APIs.
def to_dict(self): | ||
planet_to_dict = {} | ||
planet_to_dict["id"] = self.id | ||
planet_to_dict["name"] = self.name | ||
planet_to_dict["description"] = self.description | ||
planet_to_dict["moons"] = self.moons | ||
|
||
return planet_to_dict | ||
|
||
@classmethod | ||
def from_dict(cls, planet_data): | ||
new_planet = Planet(name=planet_data["name"], description=planet_data["description"], moons=planet_data["moons"]) | ||
return new_planet | ||
|
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.
Good helper methods
def validate_model(cls, model_id): | ||
try: | ||
model_id = int(model_id) | ||
except: | ||
abort(make_response({"message":f"{cls.__name__} {model_id} invalid"}, 400)) | ||
|
||
model = cls.query.get(model_id) | ||
|
||
if not model: | ||
abort(make_response({"message":f"{cls.__name__} {model_id} not found"}, 404)) | ||
|
||
return model | ||
|
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.
great helper function
planets_bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
|
||
@planets_bp.route("", methods=["POST"]) | ||
def create_new_planet(): |
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 may want to include validation on the request body for this post request. Basically check to ensure that the required fields are present.
return make_response(f"Planet {new_planet.name} successfully created", 201) | ||
|
||
@planets_bp.route("", methods=["GET"]) | ||
def read_all_planets(): |
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.
Nice use of the query params.
@@ -0,0 +1,42 @@ | |||
def test_get_all_planets_with_no_records(client): |
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 also include a get_all _planets test with several planets in the DB as well.
No description provided.