-
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
Sea Turtles - Georgia A & Lili P #9
base: main
Are you sure you want to change the base?
Conversation
…ion, make a Planet instance method to_dict
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.
Looks good so far! I'd just suggest a little reorganization of the route file to its own folder, and keep pushing yourselves to try python syntax like list comprehensions when you have the chance.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
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.
👍
app/routes.py
Outdated
bp = Blueprint("planets_bp", __name__, url_prefix="/planets") | ||
|
||
|
||
def validate_planet(id): |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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
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.
Nice job finishing up solar system. This is a great foundation to keep working from for Task List and more! Keep an eye out for additional refactoring and error handling as you continue to work with APIs.
@@ -0,0 +1,68 @@ | |||
from unicodedata import name |
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.
Looks like a stray import from VS Code got in here. Thanks, VS Code!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
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,34 @@ | |||
"""adds Planet model |
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.
👍 Nice migration message
|
||
|
||
@planets_bp.route("", methods=["GET"]) | ||
def 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.
👍
|
||
|
||
@planets_bp.route("/<planet_id>", methods=["GET"]) | ||
def get_planet_by_id(planet_id): |
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.
👍
planet.name = request_body["name"] | ||
planet.description = request_body["description"] | ||
planet.has_life = request_body["has_life"] |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, too, make sure to jsonify this.
No description provided.