Skip to content

Conversation

mkeefer17
Copy link

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work Marie & Luciane, this looks good. I made some minor suggestions, let me know if you have any questions via slack.

Comment on lines +9 to +24
def to_dict(self):
planet_as_dict = {}
planet_as_dict["id"] = self.id
planet_as_dict["name"] = self.name
planet_as_dict["description"] = self.description
planet_as_dict["size"] = self.size

return planet_as_dict

@classmethod
def from_dict(cls, planet_data):
new_planet = Planet(name=planet_data["name"],
description=planet_data["description"],
size=planet_data["size"])
return new_planet

Choose a reason for hiding this comment

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

Love the helper methods

@@ -0,0 +1,23 @@
from app import db

Choose a reason for hiding this comment

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

Just noticed you have an app/model/planet.py and app/models/planet.py So.... one isn't needed.

planets_bp = Blueprint("planets", __name__, url_prefix="/planets")

#helper functions
def validate_model(cls, model_id):

Choose a reason for hiding this comment

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

Great helper

@planets_bp.route("", methods=["POST"])
def create_planet():
request_body = request.get_json()
new_planet = Planet.from_dict(request_body)

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 ensure required fields are present would be nice.

Comment on lines +56 to +58
planet.name = request_body["name"]
planet.description = request_body["description"]
planet.size = request_body["size"]

Choose a reason for hiding this comment

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

Again some validation in the request body would be nice.

@@ -0,0 +1,47 @@
# get all planets and return no records
def test_get_all_planets_with_no_records(client):

Choose a reason for hiding this comment

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

Doing a get all with multiple planets would make a good test.

"size": "Big"
}

def test_create_one_planet(client):

Choose a reason for hiding this comment

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

Testing an invalid create action would make a good test as well.

return planet.to_dict()

@planets_bp.route("/<planet_id>", methods=["PUT"])
def update_planet(planet_id):

Choose a reason for hiding this comment

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

Just noting this route is untested

return make_response(jsonify(f"Planet #{planet.id} successfully updated"))

@planets_bp.route("/<planet_id>", methods=["DELETE"])
def delete_planet(planet_id):

Choose a reason for hiding this comment

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

Just noting this route is untested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants