-
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 Team 11: Selam & Kindra #23
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.
Nice work Selam & Kinda. You hit the learning goals here. Well done. I left some minor comments and let me know if you have questions via slack.
def to_dict(self): | ||
planet_dict = {} | ||
planet_dict["id"] = self.id | ||
planet_dict["name"] = self.name | ||
planet_dict["description"] = self.description | ||
planet_dict["diameter"] = self.diameter | ||
return planet_dict | ||
|
||
@classmethod | ||
def from_dict(cls, planet_data): | ||
new_planet = Planet(name=planet_data["name"], | ||
description=planet_data["description"], | ||
diameter=planet_data["diameter"] | ||
) | ||
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.
Nice helper methods.
def validate_planet(cls, planet_id): | ||
try: | ||
planet_id = int(planet_id) | ||
except: | ||
abort(make_response({"message":f"{cls.__name__} {planet_id} invalid"}, 400)) | ||
|
||
planet = cls.query.get(planet_id) | ||
|
||
if not planet: | ||
abort(make_response({"message":f"{cls.__name__} {planet_id} not found"}, 404)) | ||
|
||
return 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.
Nice validation function
|
||
@planet_bp.route("", methods=["POST"]) | ||
def create_planet(): | ||
request_body = request.get_json() |
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.
Doing some validation on the request body (to make sure it has the required fields) would be appropriate.
def update_planet(planet_id): | ||
planet = validate_planet(Planet, planet_id) | ||
|
||
request_body = request.get_json() |
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.
Doing validation on the request body would be appropriate here.
} | ||
] | ||
|
||
def test_create_one_planet(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.
Testing the create action with an invalid request body is also appropriate.
return planet.to_dict() | ||
|
||
@planet_bp.route("/<planet_id>", methods=["DELETE"]) | ||
def delete_planet(planet_id): |
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.
Just noting this is untested.
return make_response(jsonify(f"Planet #{planet.id} successfully deleted!")) | ||
|
||
@planet_bp.route("/<planet_id>", methods=["PUT"]) | ||
def update_planet(planet_id): |
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.
Just noting this is untested.
Waves 1 & 2