C-17 Sharks Raha-Jamie #29
Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Raha and Jamie! Great work in finishing solar system. I have provided comments on small ways y'all can improve the API.
🦈 ✨
| migrate = Migrate() | ||
| load_dotenv() | ||
|
|
||
| def create_app(test_config=None): |
| @@ -0,0 +1,15 @@ | |||
| from app import db | |||
|
|
|||
| class Planet(db.Model): | |||
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from .routes.routes import planet_bp |
There was a problem hiding this comment.
We may want to change the import to planet_routes.py since it contains the nested route for moons.
And planet_routes is a much better file name to hold all the planet routes.
| from .routes.routes import planet_bp | |
| from .routes.planet_routes import planet_bp |
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| moons = db.Column(db.Integer) |
There was a problem hiding this comment.
Should we be able to make planets with no name? To prevent our API from creating a planet with a null name, we can set that column to nullable = False.
Additionally, shouldn't moons be a relationship instead of a column? I also noticed that y'all forgot to add back_populates to Planet. Remember that if we use back_populates, we need to add it to both the parent and child models.
name = db.Column(db.String, nullable=False)
description = db.Column(db.String)
moons = db.Relationship('Moon', back_populates='planet')| def to_json(self): | ||
| return { | ||
| "id": self.id, | ||
| "name": self.name, | ||
| "description": self.description, | ||
| "moons": self.moons | ||
| } |
| @planets_bp.route("/<planet_id>/moons", methods=["POST"]) | ||
| def write_moon_to_planet(planet_id): | ||
| planet = validate_planet(planet_id) | ||
| request_body = request.get_json() | ||
| new_moon = Moon(name=request_body["name"], planet=planet) | ||
|
|
||
| db.session.add(new_moon) | ||
| db.session.commit() | ||
|
|
||
| return make_response(jsonify(f"Moon {new_moon.name} of the planet {new_moon.planet.name} successfully created"), 201) No newline at end of file |
There was a problem hiding this comment.
The previous comment about not needing make_response can also apply here.
| @@ -0,0 +1,75 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
Comments from planet_routes.py can also apply to this file.
| def two_saved_planets(app): | ||
| Mercury = Planet(name = "Mercury", description = "This is the first planet", moons = 2) | ||
| Venus = Planet(name = "Venus", description = "named after the Roman goddess of love and beauty", moons = 0) | ||
|
|
||
| db.session.add_all([Mercury,Venus]) | ||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
👍
Nice work! My one critique is about Mercury and Venus variables looking like classes. Capitalizing the first letter in a name is reserved for class names like, Planet. To avoid confusion and follow pep8 guidelines, let's continue to practice lowercase for our variable names.
| @@ -0,0 +1,37 @@ | |||
|
|
|||
|
|
||
| # moons_bp = Blueprint("moons", __name__, url_prefix="/moons") | ||
|
|
||
| # do we even need this No newline at end of file |
There was a problem hiding this comment.
Nope, you don't need this file unless you're planning on adding future enhancements for moons 🌚 ✨
| # Helper function: | ||
| from flask import abort, make_response, jsonify | ||
| from app.models.planet import Planet | ||
|
|
||
| def validate_planet(planet_id): | ||
| try: | ||
| planet_id = int(planet_id) | ||
| except: | ||
| abort(make_response(jsonify(f"planet {planet_id} invalid"), 400)) | ||
|
|
||
| planet = Planet.query.get(planet_id) | ||
| if not planet: | ||
| abort(make_response(jsonify(f"planet {planet_id} not found"), 404)) | ||
| return planet No newline at end of file |
There was a problem hiding this comment.
Helpers look great! 👍 We may want to consider making the validation code as generic as possible to process any class and id or renaming this file to helper_planet_routes or planet_routes_helper. Either would be helpful if we expand this project to contain more routes for moons, suns, etc.
A more generic validation helper method would look like:
from flask import make_response, abort
def validate_object(cls, id):
try:
id = int(id)
except:
return abort(make_response({"message": f"{cls} {id} is invalid"}, 400))
obj = cls.query.get(id)
if not obj:
abort(make_response({"message": f"{cls} {id} not found"},404))
return obj3 participants
No description provided.