Skip to content

Conversation

im-stacy
Copy link

solar-system-api part 1

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 Tanil & Stacy, you hit the learning goals here. Nice work testing and writing validations. I left some minor comments. Let me know if you have questions.

Comment on lines +11 to +31
def to_dict(self):
return({
"id": self.id,
"name": self.name,
"color": self.color,
"description": self.description
})
@classmethod
def from_json(cls, req_body):
return cls(
name = req_body["name"],
color = req_body["color"],
description = req_body["description"])

def update(self, req_body):
try:
self.name = req_body["name"]
self.color = req_body["color"]
self.description = req_body["description"]
except KeyError as error:
raise error

Choose a reason for hiding this comment

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

Great helper methods

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

# HELPER FUNCTION
def validate_id(class_obj, id):

Choose a reason for hiding this comment

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

Nice helper method

# CREATE RESOURCE
@planets_bp.route("", methods = ["POST"])
def create_planet():
request_body = request.get_json()

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 here would be nice, just to check if it has the required fields.

Comment on lines +69 to +72
try:
planet.update(request_body)
except KeyError as error:
return make_response({'message': f"Missing attribute: {error}"}, 400)

Choose a reason for hiding this comment

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

This is good validation, something similar could be done in the post request. It should also be tested

Comment on lines +43 to +51
assert response_body[0]["name"] == "foo1"
assert response_body[1]["name"] == "foo2"
assert response_body[2]["name"] == "foo3"
assert response_body[0]["color"] == "bar1"
assert response_body[1]["color"] == "bar2"
assert response_body[2]["color"] == "bar3"
assert response_body[0]["description"] == "foobar1"
assert response_body[1]["description"] == "foobar2"
assert response_body[2]["description"] == "foobar3"

Choose a reason for hiding this comment

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

You could also do something like:

Suggested change
assert response_body[0]["name"] == "foo1"
assert response_body[1]["name"] == "foo2"
assert response_body[2]["name"] == "foo3"
assert response_body[0]["color"] == "bar1"
assert response_body[1]["color"] == "bar2"
assert response_body[2]["color"] == "bar3"
assert response_body[0]["description"] == "foobar1"
assert response_body[1]["description"] == "foobar2"
assert response_body[2]["description"] == "foobar3"
assert response_body == [ {
"name": "foo1",
"color": "bar1",
"description": "foobar1",
},
{
"name": "foo2",
"color": "bar2",
"description": "foobar2",
}
...




def test_create_planet_can_create_planet_in_empty_db(client):

Choose a reason for hiding this comment

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

You should also test an invalid create planet request.


# DELETE RESOURCE
@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 a note that this 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