Sea Turtles - Georgia A & Lili P #9
Conversation
…ion, make a Planet instance method to_dict
app/routes.py
Outdated
| @@ -1,2 +1,53 @@ | |||
| from flask import Blueprint | |||
| from flask import Blueprint, jsonify, abort, make_response | |||
There was a problem hiding this comment.
Consider moving this file into a routes folder. This is useful even when we don't have another set of routes to store there, since we'll be adding additional files, and having things grouped logically can help us find our way around our code.
app/__init__.py
Outdated
| from .routes import bp | ||
| app.register_blueprint(bp) |
There was a problem hiding this comment.
👍
If you move the routes.py file this will need to change a little.
app/routes.py
Outdated
|
|
||
| class Planet: | ||
| def __init__(self, id, name, description, life): |
There was a problem hiding this comment.
👍
Consider renaming life to has_life. This helps us know to expect a boolean value. Boolean variables/attributes are often named as has_something or is_something. Essentially, we try to phrase the name so that it implies that this is a yes/no, or true/false value.
app/routes.py
Outdated
| self.description = description | ||
| self.life = life | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
👍
Instance method for building a dictionary from an instance looks good! (Again, consider renaming life.)
app/routes.py
Outdated
| planets = [ | ||
| Planet(1, "Earth", "Best planet", True), | ||
| Planet(2, "Saturn", "Got a ring on it", False), | ||
| Planet(3, "Mars", "We want to go there", False) | ||
| ] |
There was a problem hiding this comment.
We'll be getting rid of this collection soon, but consider using named arguments here to help this be a little more self-documenting about what the various values are. Id and name are pretty clear, but it might be harder to tell that the second string is a description, and I'd be hard pressed to guess that the final boolean was about whether there was life there.
app/routes.py
Outdated
| Planet(3, "Mars", "We want to go there", False) | ||
| ] | ||
|
|
||
| bp = Blueprint("planets_bp", __name__, url_prefix="/planets") |
app/routes.py
Outdated
| bp = Blueprint("planets_bp", __name__, url_prefix="/planets") | ||
|
|
||
|
|
||
| def validate_planet(id): |
There was a problem hiding this comment.
Nice decision to pull this up above the two routes. And the validation method looks good, for both detecting a bad id, as well as an id with no record.
app/routes.py
Outdated
|
|
||
|
|
||
| @bp.route("", methods=["GET"]) | ||
| def get_planets(): |
There was a problem hiding this comment.
👍
Consider using a list comprehension for generating the result list. This would be especially nice since you have the to_dict instance method.
app/routes.py
Outdated
| return jsonify(planets_list) | ||
|
|
||
| @bp.route("/<id>", methods=["GET"]) | ||
| def get_one_planet(id): |
There was a problem hiding this comment.
👍
Single planet endpoint looks good. We're able to reuse that dictionary instance method and move most of the logic into the validation method.
…refactored GET endpoint for Planets, and some other small refactoring
…eated error_message function
| @@ -0,0 +1,68 @@ | |||
| from unicodedata import name | |||
There was a problem hiding this comment.
Looks like a stray import from VS Code got in here. Thanks, VS Code!
There was a problem hiding this comment.
Also, since you moved this file into a routes folder, consider naming this file something like planet_routes.py. This is helpful when we have routes for multiple model types, and also makes the import in the app init a little more legible.
There was a problem hiding this comment.
Don't forget to add a __init__.py file in this directory. Without it, python is actually doing a slightly older style of import that can lead to trouble down the road.
| from .routes_helper import validate_planet, error_message | ||
|
|
||
|
|
||
| planets_bp = Blueprint("planets", __name__, url_prefix="/planets") |
There was a problem hiding this comment.
When a route file has only a single blueprint in it (pretty typical), consider naming the variable simply bp
| from app.routes.routes import planets_bp | ||
| app.register_blueprint(planets_bp) |
There was a problem hiding this comment.
If you renamed things according to my later suggestions, these lines could be re-written as:
from app.routes import planet_routes
app.register_blueprint(planet_routes.bp)|
|
||
| db = SQLAlchemy() | ||
| migrate = Migrate() | ||
| load_dotenv() | ||
|
|
||
| def create_app(test_config=None): |
| @@ -0,0 +1,34 @@ | |||
| """adds Planet model | |||
|
|
||
|
|
||
| @planets_bp.route("", methods=["GET"]) | ||
| def get_planets(): |
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods=["GET"]) | ||
| def get_planet_by_id(planet_id): |
| planet.name = request_body["name"] | ||
| planet.description = request_body["description"] | ||
| planet.has_life = request_body["has_life"] |
There was a problem hiding this comment.
Consider moving this to an instance method in Planet.
| error_message(f"Missing key {err}", 400) | ||
|
|
||
| db.session.commit() | ||
| return make_response(f"Planet {planet_id} successfully updated!", 200) |
There was a problem hiding this comment.
Make sure to jsonify this string, otherwise Flask won't return it as a JSON string. It will be returned as HTML. And here, if we jsonify we don't need the make_response. Also, keep in mind that 200 is the default response code, so we could leave that off.
return jsonify(f"Planet {planet_id} successfully updated!"), 200|
|
||
| db.session.delete(planet) | ||
| db.session.commit() | ||
| return make_response(f"Planet {planet_id} successfully deleted.") |
There was a problem hiding this comment.
Here, too, make sure to jsonify this.
No description provided.