-
Notifications
You must be signed in to change notification settings - Fork 61
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
Sharks- Ivana M. and Lauren K. #14
base: main
Are you sure you want to change the base?
Conversation
…created blueprint endpoint get_planets
…ed class instance method to_json
…OST /planets, GET /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.
Part 1! Nicely done! Left a few suggestions here and there, but your logic looks good
description = db.Column(db.String) | ||
order_in_ss = db.Column(db.String) | ||
|
||
def to_json(self): |
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.
👍 yay instance methods!
app/routes.py
Outdated
|
||
|
||
|
||
planets_bp = Blueprint("planets", __name__, url_prefix = "/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.
we only need this assigned once at the top!
planets_bp = Blueprint("planets", __name__, url_prefix = "/planets") |
app/routes.py
Outdated
for planet in planets: | ||
planets_response.append(planet.to_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.
we could also do this with list comprehension:
for planet in planets: | |
planets_response.append(planet.to_json()) | |
planets_response = [planet.to_json() for planet in planets] |
app/routes.py
Outdated
for planet in planets: | ||
planets_response.append(planet.to_json()) | ||
|
||
return jsonify(planets_response) |
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.
don't forget your status codes!
return jsonify(planets_response) | |
return jsonify(planets_response), 200 |
…nce method update and class method create.
…T endpoint method
…iles, populated testconfit, created fixtures, create test get_planet for empty db
…data, and create one 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.
Nicey done, Ivana and Lauren! I didn't find any red flags or any logic issues! Looks good!
"order in solar system": self.order_in_ss | ||
} | ||
|
||
def update_planet(self, update_body): |
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.
👍
self.order_in_ss = update_body["order in solar system"] | ||
|
||
@classmethod | ||
def create_planet(cls, request_body): |
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.
👍
@@ -0,0 +1,14 @@ | |||
from flask import abort, make_response |
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 job separating this out!
new_planet = cls( | ||
name=request_body['name'], | ||
description=request_body['description'], | ||
order_in_ss=request_body['order in solar system'], |
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.
careful with spaces! while this works syntactically, I don't think it's very common to see keys with spaces like this. Maybe something like order_in_ss
might look better
@@ -0,0 +1,55 @@ | |||
from app import db |
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.
👍
@@ -0,0 +1,41 @@ | |||
import pytest |
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.
👍
@@ -0,0 +1,64 @@ | |||
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.
👍
No description provided.