Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good so far! I'd just suggest a little reorganization of the route file to its own folder. Also, is it just me, or some of the indents especially wide? 🤔
app/planet_routes.py
Outdated
| @@ -0,0 +1,52 @@ | |||
| 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 .planet_routes import bp | ||
| app.register_blueprint(bp) |
There was a problem hiding this comment.
👍
If you move the planet_routes.py file this will need to change a little.
app/planet_routes.py
Outdated
| self.description = description | ||
| self.no_life = no_life | ||
|
|
||
| def to_dict(self): |
There was a problem hiding this comment.
👍
Instance method for building a dictionary from an instance looks good!
app/planet_routes.py
Outdated
| Planet(1, "Methuselah", "oldest exoplanet"), | ||
| Planet(2, "Epsilon Eridani b", "closest exoplanet, has 2 astroid belts!!"), | ||
| Planet(3, "Aquarii A", "the most suns!"), | ||
| Planet(4, "Gliese 876 b", "gas giant, big, icy moons, water might be found", False) |
There was a problem hiding this comment.
Read it here first: it's False that there's no life on Gliese 876b!
There was a problem hiding this comment.
Although we'll be getting rid of this collection soon, 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 the other params, especially the bare boolean, aren't as obvious. Having those as named arguments would make it explicit.
app/planet_routes.py
Outdated
| Planet(4, "Gliese 876 b", "gas giant, big, icy moons, water might be found", False) | ||
| ] | ||
|
|
||
| bp = Blueprint("planets", __name__, url_prefix="/planets") |
app/planet_routes.py
Outdated
| bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
|
|
||
| @bp.route("", methods=["GET"]) | ||
| def index_planets(): |
There was a problem hiding this comment.
👍
Get all looks good. I like the function name you used too. Nice use of the list comprehension and instance method to build the dictionary for the instance.
app/planet_routes.py
Outdated
|
|
||
| return jsonify(result_list) | ||
|
|
||
| def validate_planet(id): |
There was a problem hiding this comment.
👍
Validation method looks good, for both detecting a bad id, as well as an id with no record.
app/planet_routes.py
Outdated
|
|
||
|
|
||
| @bp.route("/<id>", methods=["GET"]) | ||
| def get_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.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job finishing up solar system, and adding so many additional tests! 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.
|
|
||
| return app | ||
| if not test_config: | ||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False |
There was a problem hiding this comment.
Notice that since this option is being set in both branches of the if, it could be pulled out of the branches to either before or after the if to remove the repetition.
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
Once we have an import path that leads to the model being imported (init→route→model) we don't really need this import here, but it can be handy as a sanity check.
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| life = db.Column(db.Boolean) | ||
| moons = db.Column(db.Integer) |
There was a problem hiding this comment.
Consider setting some of these columns to have nullable=false. For example, should we be able to create a planet without a name? Should life be able to be NULL rather than true or false? If there are no moons, shouldn't we require the value to be 0 rather than NULL?
| life = db.Column(db.Boolean) | ||
| moons = db.Column(db.Integer) | ||
|
|
||
| def to_dict(self): |
| ) | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data_dict): |
| result = replace_planet_safely(planet, request_body) | ||
| db.session.commit() | ||
| return jsonify(result) |
There was a problem hiding this comment.
I'd be more inclined to have the replace calls just update the contents without returning the dictionarified result. More like the following.
replace_planet_safely(planet, request_body)
db.session.commit()
return jsonify(planet.to_dict())| def upgrade_planet_with_id(id): | ||
| planet = validate_planet(id) | ||
| request_body = request.get_json() | ||
| planet_keys = request_body.keys() |
There was a problem hiding this comment.
We don't need to retrieve the keys separately here. The in operator on dictionaries checks for a key itself, so the later checks could be written as:
if "name" in request_body:
# handle name key...| if "name" in planet_keys: | ||
| planet.name = request_body["name"] | ||
| if "description" in planet_keys: | ||
| planet.description = request_body["description"] | ||
| if "life" in planet_keys: | ||
| planet.life = request_body["life"] | ||
| if "moons" in planet_keys: | ||
| planet.moons = request_body["moons"] |
There was a problem hiding this comment.
Consider moving this to an instance method in Planet.
| def test_delete_planet(client, two_planets): | ||
|
|
||
| response = client.delete("/planets/1") | ||
| response_body = response.get_data(as_text=True) |
There was a problem hiding this comment.
Rather than having to handle this response specially, I'd suggest jsonfying the string that gets returned so that it is available through get_json() like all our other responses. We are writing a JSON web api, so we should be consistent about returning JSON results.
| planet = validate_planet(id) | ||
| db.session.delete(planet) | ||
| db.session.commit() | ||
| return make_response('*** You have successfully destroyed Earth ! ***') No newline at end of file |
There was a problem hiding this comment.
Make sure to return this as JSON. If we return a bare string, Flask will assume we want to return it as HTML. Also, make sure not to hardcode the name of the planet being destroyed.
return jsonify(f'*** You have successfully destroyed {planet.name} ! ***')3 participants
No description provided.